Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID

["Marc-André Lureau" <marcandre.lureau@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8935 bytes --]

Hi

On Wed, Aug 25, 2021 at 1:39 PM Stefan Reiter <s.reiter@proxmox.com> wrote:

> It is possible to specify more than one VNC server on the command line,
> either with an explicit ID or the auto-generated ones à la "default",
> "vnc2", "vnc3", ...
>
> It is not possible to change the password on one of these extra VNC
> displays though. Fix this by adding a "display" parameter to the
> 'set_password' and 'expire_password' QMP and HMP commands.
>
> For HMP, this is a bit trickier, since at least 'set_password' already
> has the 'connected' parameter following the mandatory 'password' one, so
> we need to prefix the display ID with "id=" to allow correct parsing.
>

It's not something done with other commands afaik, feels a bit awkward (the
"connected = display"...).

Is it really necessary to add support for HMP?

With this prefix, no existing command or workflow should be affected.
>
> While rewriting the descriptions, also remove the line "Use zero to make
> the password stay valid forever." from 'set_password', I believe this was
> intended for 'expire_password', but would even be wrong there.
>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  hmp-commands.hx    | 28 +++++++++++++++-------------
>  monitor/hmp-cmds.c | 20 ++++++++++++++++++--
>  monitor/qmp-cmds.c |  9 +++++----
>  qapi/ui.json       | 12 ++++++++++--
>  4 files changed, 48 insertions(+), 21 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e01ca13ca8..0b5abcfb8a 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1541,34 +1541,36 @@ ERST
>
>      {
>          .name       = "set_password",
> -        .args_type  = "protocol:s,password:s,connected:s?",
> -        .params     = "protocol password action-if-connected",
> +        .args_type  = "protocol:s,password:s,display:s?,connected:s?",
> +        .params     = "protocol password [id=display]
> [action-if-connected]",
>          .help       = "set spice/vnc password",
>          .cmd        = hmp_set_password,
>      },
>
>  SRST
> -``set_password [ vnc | spice ] password [ action-if-connected ]``
> -  Change spice/vnc password.  Use zero to make the password stay valid
> -  forever.  *action-if-connected* specifies what should happen in
> +``set_password [ vnc | spice ] password [ id=display ] [
> action-if-connected ]``
> +  Change spice/vnc password.  *display* (must be prefixed with
> +  'id=') can be used with 'vnc' to specify which display to set the
> +  password on.  *action-if-connected* specifies what should happen in
>    case a connection is established: *fail* makes the password change
> -  fail.  *disconnect* changes the password and disconnects the
> -  client.  *keep* changes the password and keeps the connection up.
> -  *keep* is the default.
> +  fail.  *disconnect* changes the password and disconnects the client.
> +  *keep* changes the password and keeps the connection up.  *keep* is
> +  the default.
>  ERST
>
>      {
>          .name       = "expire_password",
> -        .args_type  = "protocol:s,time:s",
> -        .params     = "protocol time",
> +        .args_type  = "protocol:s,time:s,display:s?",
> +        .params     = "protocol time [id=display]",
>          .help       = "set spice/vnc password expire-time",
>          .cmd        = hmp_expire_password,
>      },
>
>  SRST
> -``expire_password [ vnc | spice ]`` *expire-time*
> -  Specify when a password for spice/vnc becomes
> -  invalid. *expire-time* accepts:
> +``expire_password [ vnc | spice ] expire-time [ id=display ]``
> +  Specify when a password for spice/vnc becomes invalid.
> +  *display* behaves the same as in ``set_password``.
> +  *expire-time* accepts:
>
>    ``now``
>      Invalidate password instantly.
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 31366e6331..30f5b2c3e3 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1546,10 +1546,20 @@ void hmp_set_password(Monitor *mon, const QDict
> *qdict)
>  {
>      const char *protocol  = qdict_get_str(qdict, "protocol");
>      const char *password  = qdict_get_str(qdict, "password");
> +    const char *display = qdict_get_try_str(qdict, "display");
>      const char *connected = qdict_get_try_str(qdict, "connected");
>      Error *err = NULL;
>
> -    qmp_set_password(protocol, password, !!connected, connected, &err);
> +    if (display && strncmp(display, "id=", 3)) {
> +        connected = display;
> +        display = NULL;
> +    } else if (display) {
> +        /* skip "id=" */
> +        display = display + 3;
> +    }
> +
> +    qmp_set_password(protocol, password, !!connected, connected,
> !!display,
> +                     display, &err);
>      hmp_handle_error(mon, err);
>  }
>
> @@ -1557,9 +1567,15 @@ 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");
> +    const char *display = qdict_get_try_str(qdict, "display");
>      Error *err = NULL;
>
> -    qmp_expire_password(protocol, whenstr, &err);
> +    if (display && !strncmp(display, "id=", 3)) {
> +        /* skip "id=" */
> +        display = display + 3;
> +    }
> +
> +    qmp_expire_password(protocol, whenstr, !!display, display, &err);
>      hmp_handle_error(mon, err);
>  }
>
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index f7d64a6457..a9ded90a41 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -165,7 +165,8 @@ void qmp_system_wakeup(Error **errp)
>  }
>
>  void qmp_set_password(const char *protocol, const char *password,
> -                      bool has_connected, const char *connected, Error
> **errp)
> +                      bool has_connected, const char *connected,
> +                      bool has_display, const char *display, Error **errp)
>  {
>      int disconnect_if_connected = 0;
>      int fail_if_connected = 0;
> @@ -198,7 +199,7 @@ 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);
> +        rc = vnc_display_password(has_display ? display : NULL, password);
>      } else {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
>                     "'vnc' or 'spice'");
> @@ -211,7 +212,7 @@ void qmp_set_password(const char *protocol, const char
> *password,
>  }
>
>  void qmp_expire_password(const char *protocol, const char *whenstr,
> -                         Error **errp)
> +                         bool has_display, const char *display, Error
> **errp)
>  {
>      time_t when;
>      int rc;
> @@ -232,7 +233,7 @@ void qmp_expire_password(const char *protocol, const
> char *whenstr,
>          }
>          rc = qemu_spice.set_pw_expire(when);
>      } else if (strcmp(protocol, "vnc") == 0) {
> -        rc = vnc_display_pw_expire(NULL, when);
> +        rc = vnc_display_pw_expire(has_display ? display : NULL, when);
>      } else {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "protocol",
>                     "'vnc' or 'spice'");
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 16bf03224f..24dca811f8 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -25,6 +25,9 @@
>  #             'disconnect' to disconnect existing clients
>  #             'keep' to maintain existing clients
>  #
> +# @display: In case of VNC, the id of the display where the password
> +#           should be changed. Defaults to the first.
> +#
>  # Returns: - Nothing on success
>  #          - If Spice is not enabled, DeviceNotFound
>  #
> @@ -38,7 +41,8 @@
>  #
>  ##
>  { 'command': 'set_password',
> -  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +  'data': {'protocol': 'str', 'password': 'str', '*connected': 'str',
> +           '*display': 'str'} }
>
>  ##
>  # @expire_password:
> @@ -54,6 +58,9 @@
>  #        - '+INT' where INT is the number of seconds from now (integer)
>  #        - 'INT' where INT is the absolute time in seconds
>  #
> +# @display: In case of VNC, the id of the display where the password
> +#           should be set to expire. Defaults to the first.
> +#
>  # Returns: - Nothing on success
>  #          - If @protocol is 'spice' and Spice is not active,
> DeviceNotFound
>  #
> @@ -71,7 +78,8 @@
>  # <- { "return": {} }
>  #
>  ##
> -{ 'command': 'expire_password', 'data': {'protocol': 'str', 'time':
> 'str'} }
> +{ 'command': 'expire_password',
> +  'data': {'protocol': 'str', 'time': 'str', '*display': 'str'} }
>
>  ##
>  # @screendump:
> --
> 2.30.2
>
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 11262 bytes --]