Re: [PATCH v8 2/3] qapi/monitor: refactor set/expire_password with enums

[Markus Armbruster <armbru@redhat.com>]

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

> From: Stefan Reiter <s.reiter@proxmox.com>
>
> '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>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> [FE: update "Since: " from 6.2 to 7.0
>      put 'keep' first in enum to ease use as a default]
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>
> v7 -> v8:
> * drop if conditionals for DisplayProtocol enum, so compilation with
>   --disable-{spice,vnc} works
>
>  monitor/hmp-cmds.c | 29 +++++++++++++++++++++++++++--
>  monitor/qmp-cmds.c | 37 ++++++++++++-------------------------
>  qapi/ui.json       | 36 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 73 insertions(+), 29 deletions(-)
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 8c384dc1b2..ff78741b75 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1398,8 +1398,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);
>  }
>  
> @@ -1408,8 +1424,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;
> +
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> +                            DISPLAY_PROTOCOL_VNC, &err);
> +    if (err) {
> +        goto out;
> +    }
>  
> -    qmp_expire_password(protocol, whenstr, &err);
> +    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 db4d186448..b6e8b57fcc 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -168,33 +168,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");
> @@ -203,10 +197,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) {
> @@ -214,7 +204,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;
> @@ -230,17 +220,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 9354f4c467..e112409211 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -9,6 +9,34 @@
>  { 'include': 'common.json' }
>  { 'include': 'sockets.json' }
>  
> +##
> +# @DisplayProtocol:
> +#
> +# Display protocols which support changing password options.

This is correct now: the enum is only used in that role.  If we ever use
DisplayReloadType for other purposes, the comment will become wrong.
Let's not worry about that now.

> +#
> +# Since: 7.0
> +#
> +##
> +{ 'enum': 'DisplayProtocol',
> +  'data': [ 'vnc', 'spice' ] }
> +
> +##
> +# @SetPasswordAction:
> +#
> +# An action to take on changing a password on a connection with active clients.
> +#
> +# @keep: maintain existing clients
> +#
> +# @fail: fail the command if clients are connected
> +#
> +# @disconnect: disconnect existing clients
> +#
> +# Since: 7.0
> +#
> +##
> +{ 'enum': 'SetPasswordAction',
> +  'data': [ 'keep', 'fail', 'disconnect' ] }
> +
>  ##
>  # @set_password:
>  #
> @@ -38,7 +66,9 @@
>  #
>  ##
>  { 'command': 'set_password',
> -  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'password': 'str',
> +            '*connected': 'SetPasswordAction' } }
>  
>  ##
>  # @expire_password:
> @@ -71,7 +101,9 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time': 'str'} }
> +{ 'command': 'expire_password',
> +  'data': { 'protocol': 'DisplayProtocol',
> +            'time': 'str' } }
>  
>  ##
>  # @screendump:

Reviewed-by: Markus Armbruster <armbru@redhat.com>