Stefan Reiter <s.rei...@proxmox.com> writes:
> 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, the display is specified using the "-d" value flag.
>
> For QMP, the schema is updated to explicitly express the supported
> variants of the commands with protocol-discriminated unions.
>
> Suggested-by: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Stefan Reiter <s.rei...@proxmox.com>
> ---
> hmp-commands.hx | 24 +++++-----
> monitor/hmp-cmds.c | 51 +++++++++++++++------
> monitor/qmp-cmds.c | 36 ++++++---------
> qapi/ui.json | 112 +++++++++++++++++++++++++++++++++++----------
> 4 files changed, 154 insertions(+), 69 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index cf723c69ac..9fbb207b35 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1514,33 +1514,35 @@ 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:-dV,connected:s?",
> + .params = "protocol password [-d 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. *action-if-connected* specifies what
> - should happen in case a connection is established: *fail* makes the
> - password change fail. *disconnect* changes the password and
> +``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected
> ]``
> + Change spice/vnc password. *display* 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.
> ERST
>
> {
> .name = "expire_password",
> - .args_type = "protocol:s,time:s",
> - .params = "protocol time",
> + .args_type = "protocol:s,time:s,display:-dV",
> + .params = "protocol time [-d 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 [ -d 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 b8abe69609..daa4a8e106 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1451,26 +1451,39 @@ 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;
> - DisplayProtocol proto;
> - SetPasswordAction conn;
>
> - proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> - DISPLAY_PROTOCOL_VNC, &err);
> + SetPasswordOptions opts = {
> + .password = g_strdup(password),
> + .u.vnc.display = NULL,
> + };
> +
> + opts.protocol = 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;
> + if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> + opts.u.vnc.has_display = !!display;
> + opts.u.vnc.display = g_strdup(display);
> + } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
> + opts.u.spice.has_connected = !!connected;
> + opts.u.spice.connected =
> + qapi_enum_parse(&SetPasswordAction_lookup, connected,
> + SET_PASSWORD_ACTION_KEEP, &err);
> + if (err) {
> + goto out;
> + }
> }
>
> - qmp_set_password(proto, password, !!connected, conn, &err);
> + qmp_set_password(&opts, &err);
>
> out:
> + g_free(opts.password);
> + g_free(opts.u.vnc.display);
Uh-oh...
For DISPLAY_PROTOCOL_SPICE, we execute
.u.vnc.display = NULL,
opts.u.spice.has_connected = !!connected;
opts.u.spice.connected =
qapi_enum_parse(&SetPasswordAction_lookup, connected,
SET_PASSWORD_ACTION_KEEP, &err);
opts.u.vnc.has_display = !!display;
g_free(opts.u.vnc.display);
The assignments to opts.u.spice.has_connected and opts.u.spice_connected
clobber opts.u.vnc.display.
The simplest fix is to pass @display directly. Precedence:
hmp_drive_mirror().
Do the same for @password, of course.
> hmp_handle_error(mon, err);
> }
>
> @@ -1478,18 +1491,30 @@ 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;
> - DisplayProtocol proto;
>
> - proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> - DISPLAY_PROTOCOL_VNC, &err);
> + ExpirePasswordOptions opts = {
> + .time = g_strdup(whenstr),
> + .u.vnc.display = NULL,
> + };
> +
> + opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> + DISPLAY_PROTOCOL_VNC, &err);
> if (err) {
> goto out;
> }
>
> - qmp_expire_password(proto, whenstr, &err);
> + if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> + opts.u.vnc.has_display = !!display;
> + opts.u.vnc.display = g_strdup(display);
> + }
Use of -d with spice are silently ignored. Do we care? Same for
hmp_set_password() above.
> +
> + qmp_expire_password(&opts, &err);
>
> out:
> + g_free(opts.time);
> + g_free(opts.u.vnc.display);
No uh-oh here, since opts.u.vnc is actually the only member of opts.u.
Still, let's pass @time and @display directly for consistency and
robustness.
> hmp_handle_error(mon, err);
> }
>
[...]
