[2/2] QMP: Introduce Human Monitor passthrough command

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:

 o need to store monitor related data (eg. getfd)
 o read data from the user (eg. cont when the block device is
   encrypted)

TODO: Create a blacklist for those bad commands or just let
      them fail? (assuming they won't blowup, of course)
TODO: Maybe a command like 'cpu' requires a blacklist

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   34 ++++++++++++++++++++++++++++++++++
 qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)

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:
>
>  o need to store monitor related data (eg. getfd)

Could you explain the problem here?

>  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?

>
> 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.

> TODO: Maybe a command like 'cpu' requires a blacklist

What's the problem with "cpu"?

> 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!

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!
>