[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] hmp: add console support for ringbuf backen
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] hmp: add console support for ringbuf backend |
Date: |
Mon, 9 Sep 2013 16:27:04 -0400 |
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