Re: [Qemu-devel] [PATCH 2/2] QMP: Introduce Human Monitor passthrough command

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 10 Nov 2010 11:03:56 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This command allows QMP clients to execute HMP commands, please
> > check its documentation in the hmp-commands.hx file for usage
> > information.
> >
> > Please, also note that not all HMP commands can be executed this
> > way, in special commands that:

This changed a bit in v1, care to review it instead? I will answer your
questions but skip the code comments.

> >
> >  o need to store monitor related data (eg. getfd)
> 
> Could you explain the problem here?

IIUC, we store received file-descriptors in the Monitor struct, however the
passthrough is totally stateless so we don't have where to store them.

> >  o read data from the user (eg. cont when the block device is
> >    encrypted)
> 
> The human monitor does I/O on a character device.  Your human monitor
> captures output, and sends it back over QMP.  Input could be sent over
> QMP, too.  Just not interactively, as all of the input needs to be sent
> along with the command.
> 
> What can't work is funky terminal stuff such as readline, but the human
> monitor already degrades gracefully when run on a non-terminal character
> device, doesn't it?

Yes. We could make this kind of command work by adding an input buffer to
the MemoryDriver, but I'm not sure if the additional complexity is worth it.

> > TODO: Create a blacklist for those bad commands or just let
> >       them fail? (assuming they won't blowup, of course)
> 
> We need to make sure "bad" commands fail cleanly anyway, so things don't
> explode when we get the blacklist slightly wrong.

Sure, as far as I could test it, no command will explode, if it happens
it's a bug of course.

> > TODO: Maybe a command like 'cpu' requires a blacklist
> 
> What's the problem with "cpu"?

Fixed in v1.

> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  monitor.c       |   34 ++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+), 0 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 260cc02..a0df098 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -490,6 +490,40 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
> >      return 0;
> >  }
> >  
> > +static void handle_user_command(Monitor *mon, const char *cmdline);
> > +
> > +static void hmp_call(Monitor *mon, const char *cmdline)
> > +{
> > +    Monitor *old_mon = cur_mon;
> > +
> > +    cur_mon = mon;
> > +    handle_user_command(mon, cmdline);
> > +    cur_mon = old_mon;
> > +}
> 
> Do you expect more users of this function?
> 
> > +
> > +static int do_hmp_passthrough(Monitor *mon, const QDict *params,
> > +                              QObject **ret_data)
> > +{
> > +    QString *qs;
> > +    Monitor hmp;
> > +    CharDriverState bufchr;
> > +
> > +    memset(&hmp, 0, sizeof(hmp));
> > +    hmp.chr = &bufchr;
> > +    qemu_chr_init_buffered(hmp.chr);
> > +
> > +    hmp_call(&hmp, qdict_get_str(params, "command-line"));
> > +
> > +    qs = qemu_chr_buffered_to_qs(hmp.chr);
> > +    if (qs) {
> > +        *ret_data = QOBJECT(qs);
> > +    }
> 
> If the command produces no output, qemu_chr_buffered_to_qs() returns
> null (which I don't like, see there), and *ret_data remains untouched.
> Works for me.
> 
> > +
> > +    qemu_chr_close_buffered(hmp.chr);
> > +
> > +    return 0;
> > +}
> > +
> >  static int compare_cmd(const char *name, const char *list)
> >  {
> >      const char *p, *pstart;
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 793cf1c..29a6048 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -761,6 +761,38 @@ Example:
> >  
> >  Note: This command must be issued before issuing any other command.
> >  
> > +EQMP
> > +
> > +    {
> > +        .name       = "hmp_passthrough",
> > +        .args_type  = "command-line:s",
> > +        .params     = "",
> > +        .help       = "",
> > +        .user_print = monitor_user_noop,
> > +        .mhandler.cmd_new = do_hmp_passthrough,
> > +    },
> > +
> > +SQMP
> > +hmp_passthrough
> 
> The temptation to call this "human_passthrough" would be well-nigh
> irresistible for me... can't resist cheap puns ;)
> 
> > +---------------
> > +
> > +Execute a Human Monitor command.
> > +
> > +Arguments: 
> > +
> > +- command-line: the command name and its arguments, just like the
> > +                Human Monitor's shell (json-string)
> > +
> > +Example:
> > +
> > +-> { "execute": "hmp_passthrough", "arguments": { "command-line": "info version" } }
> > +<- { "return": "0.13.50\r\n" }
> > +
> > +Note: The Human Monitor is NOT a stable interface, this means that command
> > +      names, arguments and responses can change or be removed at ANY time.
> > +      Applications that rely on long term stability guarantees should NOT
> > +      use this command.
> > +
> >  3. Query Commands
> >  =================
> 
> I'm pleasantly surprised how self-contained and simple this feature
> turned out to be.  Nice work!
>