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

[John Snow <jsnow@redhat.com>]

On 1/27/20 3:43 PM, Peter Krempa wrote:
> 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.
> 

Thanks for the context!

>>
>> 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.
> 

As thought.

> 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.
> 

Yes, noted. I wonder where userfaultfd write support is these days...

> 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
> 

Thank you for the report from libvirtville :)

--js