Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups

[Markus Armbruster]

Peter Krempa <address@hidden> writes:

> On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote:
>> 
>> 
>> On 1/27/20 5:36 AM, Maxim Levitsky wrote:
>> > This patch series is bunch of cleanups
>> > to the hmp monitor code.
>> > 
>> > This series only touched blockdev related hmp handlers.
>> > 
>> > No functional changes expected other that
>> > light error message changes by the last patch.
>> > 
>> > This was inspired by this bugzilla:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=1719169
>> > 
>> > Basically some users still parse hmp error messages,
>> > and they would like to have them prefixed with 'Error:'
>> > 
>> 
>> HMP isn't meant to be parsed. It's explicitly *not* API or ABI. I do
>> like consistency in my UIs (it's useful for human eyes, too), but I'd
>> like to know more about the request.
>
> That's true as long as there's an stable replacement ... see below.
>
>> 
>> Is this request coming from libvirt? Can we wean them off of this
>> interface? What do they need as a replacement?
>
> There are 5 commands that libvirt still has HMP interfaces for:
>
> drive_add
> drive_del
>
> savevm
> loadvm
> delvm
>
> From upstream point of view there's no value in adding the 'error'
> prefix to drive_add/drive_del as libvirt now uses blockdev-add/del QMP
> command instead which have implicit error propagation.
>
> There are no replacements for the internal snapshot commands, but they
> reported the 'error' prefix for some time even before this series.
>
> Said that, please don't break savevm/loadvm/delvm until a QMP
> replacement is added.

Replacements have been proposed several times, but they went nowhere
because they replicated the HMP commands' design flaws in QMP.  Here's
one that got some design analysis in its review, by yours truly:
https://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg03593.html

Sane QMP replacements are certainly possible, but so far nobody has
cared enough to pitch in the work.

> The bug was reported at the time when libvirt didn't use blockdev yet,
> but at this point it's pointless from our side. This wouldn't even fix
> the scenario when old (pre-5.10) libvirt would use new qemu because the
> drive-add handler never checked the error prefix.
>
> [1] 
> https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD

Thanks!