| Message ID | 1364933897-25803-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/02/2013 02:18 PM, Luiz Capitulino wrote: > The Memory chardev driver was added because, as the Monitor's output > buffer was static, we needed a way to accumulate the output of an > HMP commmand when ran by human-monitor-command. > > However, the Monitor's output buffer is now dynamic, so it's possible > for the human-monitor-command to use it instead of the Memory chardev > driver. > > This commit does that change, but there are two important > obversations about it: s/obversations/observations/ > > 1. We need a way to signal to the Monitor that it shouldn't call > chardev functions when flushing its output. This is done > by adding a new flag to the Monitor object called skip_flush > (which is set to true by qmp_human_monitor_command()) > > 2. The current code has buffered semantics: QMP clients will > only see a command's output if it flushes its output with > a new-line character. This commit changes this to unbuffered, > which means that QMP clients will see a command's output > whenever the command prints anything. Ultimately, libvirt will still buffer until it has a complete JSON reply, even if portions of that reply are being sent unbuffered now where they were previously buffered until newlines. At any rate, I guess you could do some testing with libvirt, such as: virsh qemu-monitor-command $dom --hmp info and checking that libvirt isn't confused by the new output timing behavior. > > I don't think this will matter in practice though, as I believe > all HMP commands print the new-line character anyway. Just looking at it from libvirt's point of view, the few HMP commands that libvirt.so relies on when talking to qemu 1.4 are: set_cpu drive_add drive_del savevm loadvm delvm inject-nmi sendkey [libvirt-qemu.so exposes human-monitor-command as a development aid, via 'virsh qemu-monitor-command', but this is explicitly unsupported and relegated to a separate library instead of libvirt.so for a reason] I chose to look at 'delvm'; that command always has a newline ending any error message, but on the success case, it looked like there was no output at all. But once again, since the command is only being used via the QMP 'human-monitor-command' wrapper, and libvirt is only checking for known errors and declaring success if none of the error strings are present, I think it will still work. I haven't actually tested it, though. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > monitor.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) Even without an explicit test on my part, this looks sane enough that I don't mind if you use: Reviewed-by: Eric Blake <eblake@redhat.com>
On Tue, 02 Apr 2013 14:44:44 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/02/2013 02:18 PM, Luiz Capitulino wrote: > > The Memory chardev driver was added because, as the Monitor's output > > buffer was static, we needed a way to accumulate the output of an > > HMP commmand when ran by human-monitor-command. > > > > However, the Monitor's output buffer is now dynamic, so it's possible > > for the human-monitor-command to use it instead of the Memory chardev > > driver. > > > > This commit does that change, but there are two important > > obversations about it: > > s/obversations/observations/ Fixed. > > 1. We need a way to signal to the Monitor that it shouldn't call > > chardev functions when flushing its output. This is done > > by adding a new flag to the Monitor object called skip_flush > > (which is set to true by qmp_human_monitor_command()) > > > > 2. The current code has buffered semantics: QMP clients will > > only see a command's output if it flushes its output with > > a new-line character. This commit changes this to unbuffered, > > which means that QMP clients will see a command's output > > whenever the command prints anything. > > Ultimately, libvirt will still buffer until it has a complete JSON > reply, even if portions of that reply are being sent unbuffered now > where they were previously buffered until newlines. At any rate, I > guess you could do some testing with libvirt, such as: > > virsh qemu-monitor-command $dom --hmp info > > and checking that libvirt isn't confused by the new output timing behavior. Ok, I can test libvirt, but I think my explanation wasn't good enough. The buffered output I'm referring to isn't the JSON replies, but the HMP command's output carried in the JSON reply. For example, if you try "info version": { "execute": "human-monitor-command", "arguments": { "command-line": "info version" } } {"return": "1.4.50\r\n"} We can see that the 'return' dict contains the output as the hmp_info_version() command printed with monitor_printf() and that it ends with \n\r. Now, if we drop the ending new-line from the current code you'll get: { "execute": "human-monitor-command", "arguments": { "command-line": "info version" } } {"return": ""} That happens because the current code is buffered and the new-line character flushes the output. If the output is not flushed it's lost, because the buffer is destroyed when qmp_human_monitor_command() returns. This commit changes it to unbuffered, meaning that we'll get the output even if the command being run by human-monitor-command doesn't flush the output with a new-line. In theory, we could get broken output like say: {"return": "1."} In practice, I think that all HMP commands flush the output anyway, otherwise we'd have caught this on human-monitor-command already because we wouldn't get any output with the current code... Thinking about it now the current behavior seems to be worse :) And let me re-enforce that I'm doing this change to avoid having two dynamic buffers (Monitor's and human-monitor-command's). > > I don't think this will matter in practice though, as I believe > > all HMP commands print the new-line character anyway. > > Just looking at it from libvirt's point of view, the few HMP commands > that libvirt.so relies on when talking to qemu 1.4 are: > > set_cpu > drive_add > drive_del > savevm > loadvm > delvm > inject-nmi > sendkey > > [libvirt-qemu.so exposes human-monitor-command as a development aid, via > 'virsh qemu-monitor-command', but this is explicitly unsupported and > relegated to a separate library instead of libvirt.so for a reason] > > I chose to look at 'delvm'; that command always has a newline ending any > error message, but on the success case, it looked like there was no > output at all. Yes, non-info commands usually don't print anything. > But once again, since the command is only being used via > the QMP 'human-monitor-command' wrapper, and libvirt is only checking > for known errors and declaring success if none of the error strings are > present, I think it will still work. I haven't actually tested it, though. I'll test libvirt before sending a pull request. > > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > monitor.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > Even without an explicit test on my part, this looks sane enough that I > don't mind if you use: > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
diff --git a/monitor.c b/monitor.c index 8712c53..b4bda77 100644 --- a/monitor.c +++ b/monitor.c @@ -188,6 +188,7 @@ struct Monitor { int reset_seen; int flags; int suspend_cnt; + bool skip_flush; QString *outbuf; ReadLineState *rs; MonitorControl *mc; @@ -273,6 +274,10 @@ void monitor_flush(Monitor *mon) size_t len; const char *buf; + if (mon->skip_flush) { + return; + } + buf = qstring_get_str(mon->outbuf); len = qstring_get_length(mon->outbuf); @@ -675,13 +680,10 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, { char *output = NULL; Monitor *old_mon, hmp; - CharDriverState mchar; memset(&hmp, 0, sizeof(hmp)); hmp.outbuf = qstring_new(); - - qemu_chr_init_mem(&mchar); - hmp.chr = &mchar; + hmp.skip_flush = true; old_mon = cur_mon; cur_mon = &hmp; @@ -699,17 +701,14 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, handle_user_command(&hmp, command_line); cur_mon = old_mon; - if (qemu_chr_mem_osize(hmp.chr) > 0) { - QString *str = qemu_chr_mem_to_qs(hmp.chr); - output = g_strdup(qstring_get_str(str)); - QDECREF(str); + if (qstring_get_length(hmp.outbuf) > 0) { + output = g_strdup(qstring_get_str(hmp.outbuf)); } else { output = g_strdup(""); } out: QDECREF(hmp.outbuf); - qemu_chr_close_mem(hmp.chr); return output; }
The Memory chardev driver was added because, as the Monitor's output buffer was static, we needed a way to accumulate the output of an HMP commmand when ran by human-monitor-command. However, the Monitor's output buffer is now dynamic, so it's possible for the human-monitor-command to use it instead of the Memory chardev driver. This commit does that change, but there are two important obversations about it: 1. We need a way to signal to the Monitor that it shouldn't call chardev functions when flushing its output. This is done by adding a new flag to the Monitor object called skip_flush (which is set to true by qmp_human_monitor_command()) 2. The current code has buffered semantics: QMP clients will only see a command's output if it flushes its output with a new-line character. This commit changes this to unbuffered, which means that QMP clients will see a command's output whenever the command prints anything. I don't think this will matter in practice though, as I believe all HMP commands print the new-line character anyway. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- monitor.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)