Re: [Qemu-devel] [RFC 00/11]: QMP feature negotiation support

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 22 Jan 2010 11:21:22 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> A few quick questions before I dive into the patches...
>> 
>> Luiz Capitulino <address@hidden> writes:
>> 
>> >  Feature negotiation allows clients to enable QMP capabilities they are
>> > interested in using. This allows QMP to envolve without breaking old 
>> > clients.
>> >
>> >  A capability is a new QMP feature and/or protocol change which is not 
>> > part of
>> > the core protocol as defined in the QMP spec.
>> >
>> >  Feature negotiation is implemented by defining a set of rules and adding
>> > mode-oriented support.
>> >
>> >  The set of rules are:
>> >
>> > o All QMP capabilities are disabled by default
>> > o All QMP capabilities must be advertised in the capabilities array
>> > o Commands to enable/disable capabilities must be provided
>> >
>> > NOTE: Asynchronous messages are now considered a capability.
>> >
>> >  Mode-oriented support adds the following to QMP:
>> >
>> > o Two modes: handshake and operational
>> > o By default all QMP Monitors start in handshake mode
>> 
>> "By default"?  Is there a way to start a QMP monitor in another mode?
>
>  No, you think it's worth or is it just about the English?

Just about the language.

>> > o In handshake mode only commands to query/enable/disable QMP capabilities 
>> > are
>> >   allowed (there are few exceptions)
>> 
>> Note to self: check out the exception, and why we might want them.
>
>  The following handlers are handshake-only:
>
> - qmp_switch_mode
> - async_msg_enable
> - async_msg_disable

Two commands per feature?  I'd rather have a single "feature NAME
VALUE", where NAME identifies the feature, and VALUE specifies whether
to turn it on or off.

>  The following handlers are allowed to run on both modes:
>
> - query-version
> - query-qmp-mode
> - query-commands
>
>  Also, all the self-description commands (query-async-msg,
> query-errors etc) would be allowed on both modes.
>
>  So, the only handler which is not completely related to feature
> negotiation is query-version. This is only a guess, but I think
> it might be worth to let clients learn the QEMU version they are
> talking to before setting protocol features.

I'd simply announce it in the greeting, just like the features.

But I don't mind supporting selected query- commands in handshake mode,
if that's easy to do.

>> > o Clients can switch to the operational mode at any time
>> 
>> Can they switch back?  I hope not.
>
>  No, they can't. The only transition allowed is handshake -> operational.
>
>> > o In Operational mode most commands are allowed and QMP capabilities 
>> > changes
>> >   made in handshake mode take effect
>> >
>> >  Also note that each QMP Monitor has its own mode state and set of 
>> > capabilities,
>> > this means that if QEMU is started with N QMP Monitors protocol setup done 
>> > in
>> > one of them doesn't affect the others.
>> >
>> >  Session example:
>> >
>> > """
>> > {"QMP": {"capabilities": ["async messages"]}}
>> >
>> > { "execute": "query-qmp-mode" }
>> > {"return": {"mode": "handshake"}}
>> 
>> Why would clients want to query the mode?
>
>  It's more useful for testing purposes.

Just try a command that works only in one of the modes ;)

>> > { "execute": "change", "arguments": { "device": "vnc", "target": 
>> > "password", "arg": "1234" } }
>> > {"error": {"class": "QMPInvalidModeCommad", "desc": "The issued command is 
>> > invalid in this mode", "data": {}}}
>> 
>> I'd treat this like a completely unknown command.
>
>  Really? This would simplify things a bit.

Simple is good.

>> > { "execute": "async_msg_enable", "arguments": { "name": "STOP" } }
>> > {"return": {}}
>> >
>> > { "execute": "qmp_switch_mode", "arguments": { "mode": "operational" } }
>> > {"return": {}}
>> 
>> Do we envisage mode transitions other than handshake -> operational?
>
>  Today we don't, but this is about forward compatibility support right? :)
>
>  So, IMO it makes sense to have a more general command instead of
> qmp_switch_operational.

I'd say transitioning from initial feature negotiation to normal
operations is a special case, so generality is not needed here.  I'm
fine with you providing it anyway.