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

[Markus Armbruster]

Stefan Reiter <s.reiter@proxmox.com> writes:

> '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>
> ---
>  monitor/hmp-cmds.c | 17 +++++++++++++++--
>  monitor/qmp-cmds.c | 35 ++++++++++-------------------------
>  qapi/ui.json       | 37 +++++++++++++++++++++++++++++++++++--
>  3 files changed, 60 insertions(+), 29 deletions(-)
>
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index bcaa41350e..0ff18ebb3c 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1453,8 +1453,17 @@ 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 (!!connected) {
> +        conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
> +                               SET_PASSWORD_ACTION_KEEP, &err);
> +    }

@conn is left uninitialized when @connected is null.

> +
> +    qmp_set_password(proto, password, !!connected, conn, &err);
>      hmp_handle_error(mon, err);

Crash bug when more than one of the three functions fail.  See "Receive
and accumulate multiple errors (first one wins):" in qapi/error.h's big
comment.

You need to do something like

       proto = qapi_enum_parse(...);
       if (err) {
           goto out;
       }
       conn = qapi_enum_parse(...);
       if (err) {
           goto out;
       }
       qmp_set_password(...);

   out:
       hmp_handle_error(mon, err);

No need to guard the second qapi_enum_parse(): a null second argument is
fine, and the function returns its third argument then.

>  }
>  
> @@ -1463,8 +1472,12 @@ 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);
> +
> +    qmp_expire_password(proto, whenstr, &err);
>      hmp_handle_error(mon, err);

Similar crash bug.

>  }
>  
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5c0d5e116b..f1746a31fb 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -163,33 +163,26 @@ 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 if (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 +191,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 +198,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 +214,13 @@ 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) {
> +    } else if (protocol == DISPLAY_PROTOCOL_VNC) {
>          rc = vnc_display_pw_expire(NULL, when);
> -    } else {
> -        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
> -                   "'vnc' or 'spice'");
> -        return;
>      }

More robust:

       } else {
           assert(protocol == DISPLAY_PROTOCOL_VNC);
           rc = vnc_display_pw_expire(NULL, when);
       }

Same for qmp_set_password() above.

>  
>      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' ] }
> +
>  ##
>  # @set_password:
>  #
   [...]
   # @connected: how to handle existing clients when changing the
   #             password.  If nothing is specified, defaults to 'keep'
   #             'fail' to fail the command if clients are connected
   #             'disconnect' to disconnect existing clients
   #             'keep' to maintain existing clients

We explain the actions twice, in SetPasswordAction above and here.  Fine
with me.  Shortening this one would also be fine.  You might prefer not
to rock the boat at this stage ;)

> @@ -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: