Re: [Qemu-devel] [PATCH 4/4] hmp: add console support for ringbuf backend

[Luiz Capitulino]

On Mon,  2 Sep 2013 17:01:48 +0800
Lei Li <address@hidden> wrote:

> This patch add console command which would suspend the monitor,
> output the data that backed in the ringbuf backend to console
> first, and install a new readline handler to get input back to
> the ringbuf backend. Take back to the monitor once escape sequence
> 'Ctrl-]' is detected.

This command doesn't actually work (see review below), but besides
that I honestly don't fully understand its purpose.

What it seems to _try_ doing: it periodically reads from the ringbuf
buffer and print its contents. It also allow you to write to the
ringbuf (once?).

I can understand the usefulness of a console command like libvirt's,
which dumps a guest's serial output to you, but:

 1. How the reading part differs from ringbuf_read? Is it the
    timer? If it is, why not having a timer argument to ringbuf_read?

 2. How is the writing part supposed to be used? Should the user
    be able to operate a serial console for example? You sure this
    works?

 3. Did I get it wrong or you're able to write to the console
    only once?

More detailed review below, but I don't think we should move forward
before answering these questions.

> 
> Signed-off-by: Lei Li <address@hidden>
> ---
>  hmp-commands.hx |   21 ++++++++++
>  hmp.c           |  110 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h           |    1 +
>  3 files changed, 132 insertions(+), 0 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 65b7f60..286d48d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -876,6 +876,27 @@ stops because the size limit is reached.
>  ETEXI
>  
>      {
> +        .name       = "console",
> +        .args_type  = "chardev:s",
> +        .params     = "chardev",
> +        .mhandler.cmd = hmp_console,
> +    },
> +
> +STEXI
> address@hidden console @var{device}
> address@hidden console
> +Connect to the serial console from within the monitor, allow to read and
> +write data from/to ringbuf device @var{chardev}. Exit from the console and
> +return back to monitor by 'ctrl-]'.
> +
> address@hidden
> +(qemu) console foo
> +foo: data string...
> address@hidden example

That's a bad example. I think it's worth it to have a qemu command-line
example on how to use ringbuf+serial, and a more realistic example on
the command usage.

> +
> +ETEXI
> +
> +    {
>          .name       = "migrate",
>          .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
>          .params     = "[-d] [-b] [-i] uri",
> diff --git a/hmp.c b/hmp.c
> index 624ed6f..87613cc 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1521,3 +1521,113 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)
>  
>      hmp_handle_error(mon, &err);
>  }
> +
> +typedef struct ConsoleStatus
> +{
> +    QEMUTimer *timer;
> +    Monitor *mon;
> +    CharDriverState *chr;
> +} ConsoleStatus;
> +
> +enum escape_char
> +{
> +    ESCAPE_CHAR_CTRL_GS = 0x1d /* ctrl-] is used for escape */
> +};

Why is this an enum?

> +
> +static void hmp_read_ringbuf_cb(void *opaque)
> +{
> +    ConsoleStatus *status = opaque;
> +    char *data;
> +    int len;
> +    Error *err = NULL;
> +
> +    len = ringbuf_count(status->chr);

We're trying hard to not use non-QMP functions in HMP. If you need
this here, then you probably need this as a QMP command.

> +    if (len) {
> +        data = qmp_ringbuf_read(status->chr->label, len, 0, 0, &err);
> +        if (err) {
> +            monitor_printf(status->mon, "%s\n", error_get_pretty(err));
> +            error_free(err);
> +            return;

Leak status on failure?

> +        }
> +        ringbuf_print_help(status->mon, data);
> +        monitor_flush(status->mon);
> +        g_free(data);
> +        timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 
> 1000);
> +    } else {
> +        timer_del(status->timer);

If there's no data the command returns? So you try only once. Is this
the intended behavior?

> +    }
> +
> +    monitor_resume(status->mon);
> +    g_free(status);

If data was printed, the timer will run again 1s later, but you just
freed status and resumed monitor operation... Makes me think you didn't
even try this command for real? :(

> +}
> +
> +static void hmp_write_console(Monitor *mon, void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    ConsoleStatus *status;
> +
> +    status = g_malloc0(sizeof(*status));
> +    status->mon = mon;
> +    status->chr = chr;
> +    status->timer = timer_new_ms(QEMU_CLOCK_REALTIME, hmp_read_ringbuf_cb,
> +                                 status);
> +    timer_mod(status->timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME));

So, the function is called 'write console', but it actually reads from
the ringbuf?

> +}
> +
> +static void hmp_read_console(Monitor *mon, const char *data,
> +                             void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    enum escape_char console_escape = ESCAPE_CHAR_CTRL_GS;
> +    int len = strlen(data) + 1;
> +    char *tmp_buf = g_malloc0(len);
> +    int i;
> +    Error *err = NULL;
> +
> +    for (i = 0; data[i]; i++) {
> +        if (data[i] == console_escape) {
> +            monitor_read_command(mon, 1);
> +            goto out;
> +        }
> +        tmp_buf[i] = data[i];
> +    }
> +
> +    qmp_ringbuf_write(chr->label, tmp_buf, 0, 0, &err);
> +    if (err) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(err));
> +        monitor_read_command(mon, 1);
> +        error_free(err);
> +        goto out;
> +    }
> +
> +out:
> +    g_free(tmp_buf);
> +    return;
> +}
> +
> +void hmp_console(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "chardev");
> +    CharDriverState *chr;
> +    Error *err = NULL;
> +
> +    chr = qemu_chr_find(device);
> +    if (!chr) {
> +        error_set(&err, QERR_DEVICE_NOT_FOUND, device);
> +        goto out;
> +    }

You shouldn't need to do this. The qmp ringbuf commands will fail
if 'device' doesn't exist.

> +
> +    if (monitor_suspend(mon)) {
> +        monitor_printf(mon, "Failed to suspend console\n");
> +        return;
> +    }
> +
> +    hmp_write_console(mon, chr);
> +
> +    if (monitor_read_console(mon, device, hmp_read_console, chr) < 0) {
> +        monitor_printf(mon, "Connect to console %s failed\n", device);
> +    }
> +
> +out:
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 6c3bdcd..d7fb52d 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -87,5 +87,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_add(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
>  void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> +void hmp_console(Monitor *mon, const QDict *qdict);
>  
>  #endif