Re: [Qemu-devel] [PATCH v9 11/21] replay: introduce info hmp/qmp command

[Pavel Dovgalyuk]

> From: Markus Armbruster [mailto:address@hidden
> Pavel Dovgalyuk <address@hidden> writes:


> > diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
> > new file mode 100644
> > index 0000000..9956405
> > --- /dev/null
> > +++ b/replay/replay-debugging.c
> 
> The file name suggests the instruction count is just for debugging.  If
> that's true, the documentation should mention it.  Else, the file should
> be named less misleadingly, perhaps replay-control.c.

The main purpose of this file is providing an implementation of reverse
debugging methods.
Querying the rr information is mostly for debugging/problem solving too.
(until someone suggests any other manner of using these functions)

> 
> > @@ -0,0 +1,42 @@
> > +/*
> > + * replay-debugging.c
> > + *
> > + * Copyright (c) 2010-2018 Institute for System Programming
> > + *                         of the Russian Academy of Sciences.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "sysemu/replay.h"
> > +#include "replay-internal.h"
> > +#include "hmp.h"
> > +#include "monitor/monitor.h"
> > +#include "qapi/qapi-commands-replay.h"
> > +
> > +void hmp_info_replay(Monitor *mon, const QDict *qdict)
> > +{
> > +    if (replay_mode == REPLAY_MODE_NONE) {
> > +        monitor_printf(mon, "No record/replay\n");
> 
> Perhaps something like "Record/replay not active" would be friendlier.
> 
> > +    } else {
> > +        monitor_printf(mon, "%s execution '%s': current step = 
> > %"PRId64"\n",
> > +            replay_mode == REPLAY_MODE_RECORD ? "Recording" : "Replaying",
> > +            replay_get_filename(), replay_get_current_step());
> 
> You improved documentation to consistently say "instruction count".  The
> code still calls it current_step.  So does the HMP UI here.  In your
> shoes, I'd go all the way.  Consistent terminology really helps
> readers.  Advice, not demand.

Sounds reasonable. I'll probably refactor replay_get_current_step() too.

Pavel Dovgalyuk