On Mon, 2 Sep 2013 17:01:48 +0800 Lei Li <li...@linux.vnet.ibm.com> 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 <li...@linux.vnet.ibm.com> > --- > 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 > +@item console @var{device} > +@findex 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-]'. > + > +@example > +(qemu) console foo > +foo: data string... > +@end 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