Re: [Qemu-devel] [PATCH] qmp: Give saner messages related to qmp_capabilities misuse

[John Snow <jsnow@redhat.com>]

On 04/15/2015 02:31 PM, Eric Blake wrote:
> On 04/15/2015 12:13 PM, John Snow wrote:
>>
>>
>> On 04/15/2015 11:19 AM, Eric Blake wrote:
>>> Pretending that QMP doesn't understand a command merely because
>>> we are not in the right mode doesn't help first-time users figure
>>> out what to do to correct things.  Although the documentation for
>>> QMP calls out capabilities negotiation, we should also make it
>>> clear in our error messages what we were expecting.  With this
>>> patch, I now get the following transcript:
>>>
>>> $ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio -nodefaults
>>> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 2, "major": 2},
>>> "package": ""}, "capabilities": []}}
>>> {"execute":"huh"}
>>> {"error": {"class": "CommandNotFound", "desc": "The command huh has
>>> not been found"}}
>>> {"execute":"quit"}
>>> {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities
>>> negotiation with 'qmp_capabilities' before command 'quit'"}}
>>
>> Any particular reason why we should keep the "CommandNotFound" error
>> class here? Backwards compat? Inertia?
>>
>>> {"execute":"qmp_capabilities"}
>>> {"return": {}}
>>> {"execute":"qmp_capabilities"}
>>> {"error": {"class": "CommandNotFound", "desc": "Capabilities
>>> negotiation is already complete, command 'qmp_capabilities' ignored"}}
>>
>> Same here.
>
> Backwards compat. I can't prove that anyone else was relying on specific
> classes (in particular, although it is unlikely that anyone was issuing
> qmp_capabilities more than once, or cares what error class was returned,
> it IS a useful test for probing if the connection is in capability
> negotiation mode when reconnecting to a monitor after a libvirtd
> restart).  It's better to be conservative and avoid changing the error
> class (which must be reliable to machine readers) and only impact the
> error message (which is human readable and is documented to not be
> machine parseable, so we can change that at will).
>

Blast ye, Backwards Compat...

Reviewed-by: John Snow <jsnow@redhat.com>