Re: [PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums

[Markus Armbruster <armbru@redhat.com>]

Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 21.10.21 um 12:01 schrieb Stefan Reiter:
>> 'protocol' and 'connected' are better suited as enums than as strings,
>> make use of that. No functional change intended.
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>

[...]

>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index d7567ac866..15cc19dcc5 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -9,6 +9,35 @@
>>   { 'include': 'common.json' }
>>   { 'include': 'sockets.json' }
>>   +##
>> +# @DisplayProtocol:
>> +#
>> +# Display protocols which support changing password options.
>> +#
>> +# Since: 6.2
>> +#
>> +##
>> +{ 'enum': 'DisplayProtocol',
>> +  'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' },
>> +            { 'name': 'spice', 'if': 'CONFIG_SPICE' } ] }
>> +
>> +##
>> +# @SetPasswordAction:
>> +#
>> +# An action to take on changing a password on a connection with active clients.
>> +#
>> +# @fail: fail the command if clients are connected
>> +#
>> +# @disconnect: disconnect existing clients
>> +#
>> +# @keep: maintain existing clients
>> +#
>> +# Since: 6.2
>> +#
>> +##
>> +{ 'enum': 'SetPasswordAction',
>> +  'data': [ 'fail', 'disconnect', 'keep' ] }
>
> Since 'keep' should be the default, shouldn't it come first? I didn't
> find an explicit mention in the QAPI docs, but testing suggests that
> the first member will be picked. Is that correct?

Not quite.

An optional member @connected generates a pair of C struct members
@connected and @has_connected.

If @has_connected is true, the argument is present, and @connected is
its value.

If @has_connected is false, the argument is absent.  The input visitor
zeros @connected then.  Other code should as well, for robustness, but I
wouldn't bet my own money on it.

Putting the default value first in an enum makes it zero in C.  Instead
of

    has_connected ? connected : SET_PASSWORD_ACTION_KEEP

you can then write just

   connected

when you know absent values are zero.  Easier on the eyes.

A possible improvement to the QAPI schema language: optional members may
have a default value.  When given, we don't generate has_FOO.

> qmp_set_password still relies on has_connected to guard its checks
> here, but the next patch removes that, which AFAICT makes the default
> be 'fail' instead of keeping 'keep'. While it's only temporary
> breakage for VNC as the final patch in the series allows only 'keep'
> (still, should be avoided if possible), it does matter for SPICE.

Even temporary breakage should be avoided whenever practical.

[...]