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

[Fabian Ebner <f.ebner@proxmox.com>]

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>
> ---
>   monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++--
>   monitor/qmp-cmds.c | 37 ++++++++++++-------------------------
>   qapi/ui.json       | 37 +++++++++++++++++++++++++++++++++++--
>   3 files changed, 74 insertions(+), 29 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index bcaa41350e..b8abe69609 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1453,8 +1453,24 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>       const char *password  = qdict_get_str(qdict, "password");
>       const char *connected = qdict_get_try_str(qdict, "connected");
>       Error *err = NULL;
> +    DisplayProtocol proto;
> +    SetPasswordAction conn;
>   
> -    qmp_set_password(protocol, password, !!connected, connected, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        goto out;
> +    }
> +
> +    conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
> +                           SET_PASSWORD_ACTION_KEEP, &err);
> +    if (err) {
> +        goto out;
> +    }
> +
> +    qmp_set_password(proto, password, !!connected, conn, &err);
> +
> +out:
>       hmp_handle_error(mon, err);
>   }
>   
> @@ -1463,8 +1479,17 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
>       const char *protocol  = qdict_get_str(qdict, "protocol");
>       const char *whenstr = qdict_get_str(qdict, "time");
>       Error *err = NULL;
> +    DisplayProtocol proto;
>   
> -    qmp_expire_password(protocol, whenstr, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        goto out;
> +    }
> +
> +    qmp_expire_password(proto, whenstr, &err);
> +
> +out:
>       hmp_handle_error(mon, err);
>   }
>   
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5c0d5e116b..0654d7289a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -163,33 +163,27 @@ void qmp_system_wakeup(Error **errp)
>       qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
>   }
>   
> -void qmp_set_password(const char *protocol, const char *password,
> -                      bool has_connected, const char *connected, Error **errp)
> +void qmp_set_password(DisplayProtocol protocol, const char *password,
> +                      bool has_connected, SetPasswordAction connected,
> +                      Error **errp)
>   {
>       int disconnect_if_connected = 0;
>       int fail_if_connected = 0;
>       int rc;
>   
>       if (has_connected) {
> -        if (strcmp(connected, "fail") == 0) {
> -            fail_if_connected = 1;
> -        } else if (strcmp(connected, "disconnect") == 0) {
> -            disconnect_if_connected = 1;
> -        } else if (strcmp(connected, "keep") == 0) {
> -            /* nothing */
> -        } else {
> -            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
> -            return;
> -        }
> +        fail_if_connected = connected == SET_PASSWORD_ACTION_FAIL;
> +        disconnect_if_connected = connected == SET_PASSWORD_ACTION_DISCONNECT;
>       }
>   
> -    if (strcmp(protocol, "spice") == 0) {
> +    if (protocol == DISPLAY_PROTOCOL_SPICE) {
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_passwd(password, fail_if_connected,
>                                      disconnect_if_connected);
> -    } else if (strcmp(protocol, "vnc") == 0) {
> +    } else {
> +        assert(protocol == DISPLAY_PROTOCOL_VNC);
>           if (fail_if_connected || disconnect_if_connected) {
>               /* vnc supports "connected=keep" only */
>               error_setg(errp, QERR_INVALID_PARAMETER, "connected");
> @@ -198,10 +192,6 @@ void qmp_set_password(const char *protocol, const char *password,
>           /* Note that setting an empty password will not disable login through
>            * this interface. */
>           rc = vnc_display_password(NULL, password);
> -    } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
> -                   "'vnc' or 'spice'");
> -        return;
>       }
>   
>       if (rc != 0) {
> @@ -209,7 +199,7 @@ void qmp_set_password(const char *protocol, const char *password,
>       }
>   }
>   
> -void qmp_expire_password(const char *protocol, const char *whenstr,
> +void qmp_expire_password(DisplayProtocol protocol, const char *whenstr,
>                            Error **errp)
>   {
>       time_t when;
> @@ -225,17 +215,14 @@ void qmp_expire_password(const char *protocol, const char *whenstr,
>           when = strtoull(whenstr, NULL, 10);
>       }
>   
> -    if (strcmp(protocol, "spice") == 0) {
> +    if (protocol == DISPLAY_PROTOCOL_SPICE) {
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_pw_expire(when);
> -    } else if (strcmp(protocol, "vnc") == 0) {
> -        rc = vnc_display_pw_expire(NULL, when);
>       } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
> -                   "'vnc' or 'spice'");
> -        return;
> +        assert(protocol == DISPLAY_PROTOCOL_VNC);
> +        rc = vnc_display_pw_expire(NULL, when);
>       }
>   
>       if (rc != 0) {
> 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?

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.

> +
>   ##
>   # @set_password:
>   #
> @@ -38,7 +67,9 @@
>   #
>   ##
>   { 'command': 'set_password',
> -  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'password': 'str',
> +            '*connected': 'SetPasswordAction' } }
>   
>   ##
>   # @expire_password:
> @@ -71,7 +102,9 @@
>   # <- { "return": {} }
>   #
>   ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
> +{ 'command': 'expire_password',
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'time': 'str' } }
>   
>   ##
>   # @screendump: