Fabian Ebner <f.eb...@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 <arm...@redhat.com>
>> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>> Signed-off-by: Stefan Reiter <s.rei...@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.
[...]
