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

[Markus Armbruster]

Luiz Capitulino <address@hidden> 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 <address@hidden>
> ---
>  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!