Re: [Qemu-devel] [PATCH 3/4] hmp: human-monitor-command: stop using the Memory chardev driver

[Eric Blake]

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 <address@hidden>
> ---
>  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 <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature