On Mon, Sep 20, 2021 at 12:56:41PM +0200, Stefan Reiter 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, 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: Eric Blake <ebl...@redhat.com>
> Suggested-by: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Stefan Reiter <s.rei...@proxmox.com>
> ---
>
> The union schema simplifies the QMP code, but makes the HMP part a bit more
> involved. Since the parameters are practically the same, is there a way to
> just
> pass the HMP generated qdict off to the qapi parser to get the correct struct
> for calling the qmp_ methods?
Possibly, but I don't know it off-hand.
>
> hmp-commands.hx | 29 ++++----
> monitor/hmp-cmds.c | 60 +++++++++++++++-
> monitor/qmp-cmds.c | 62 ++++++-----------
> qapi/ui.json | 168 +++++++++++++++++++++++++++++++++++++--------
> 4 files changed, 235 insertions(+), 84 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 8e45bce2cd..d78e4cfc47 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1514,34 +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:-dS,connected:s?",
> + .params = "protocol password [-d display] [action-if-connected]",
Puts your new -xS of patch 2/3 to use.
> +++ b/monitor/hmp-cmds.c
> @@ -1451,10 +1451,43 @@ 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;
>
> - qmp_set_password(protocol, password, !!connected, connected, &err);
> + SetPasswordOptions opts = {
> + .password = g_strdup(password),
> + .u.vnc.display = NULL,
> + };
> +
> + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> + DISPLAY_PROTOCOL_VNC, &err);
> + if (err) {
> + hmp_handle_error(mon, err);
> + return;
> + }
> + opts.protocol = proto;
> +
> + if (proto == DISPLAY_PROTOCOL_VNC) {
> + if ((opts.u.vnc.has_display = !!display)) {
Assignment as a side-effect of the 'if' is a bit unusual in qemu
style.
> + opts.u.vnc.display = g_strdup(display);
> + }
In fact, g_strdup(NULL) does what you want; you can just write:
opts.u.vnc.has_display = !!display;
opts.u.vnc.display = g_strdup(display);
without an 'if'.
> + } else if (proto == DISPLAY_PROTOCOL_SPICE) {
> + if ((opts.u.spice.has_connected = !!connected)) {
And again.
> + opts.u.spice.connected =
> + qapi_enum_parse(&SetPasswordAction_lookup, connected,
> + SET_PASSWORD_ACTION_KEEP, &err);
> + if (err) {
> + hmp_handle_error(mon, err);
> + return;
> + }
> + }
> + }
> +
> + qmp_set_password(&opts, &err);
> + g_free(opts.password);
> + g_free(opts.u.vnc.display);
> hmp_handle_error(mon, err);
> }
>
> @@ -1462,9 +1495,32 @@ 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;
>
> - qmp_expire_password(protocol, whenstr, &err);
> + ExpirePasswordOptions opts = {
> + .time = g_strdup(whenstr),
> + .u.vnc.display = NULL,
> + };
> +
> + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> + DISPLAY_PROTOCOL_VNC, &err);
> + if (err) {
> + hmp_handle_error(mon, err);
> + return;
> + }
> + opts.protocol = proto;
> +
> + if (proto == DISPLAY_PROTOCOL_VNC) {
> + if ((opts.u.vnc.has_display = !!display)) {
> + opts.u.vnc.display = g_strdup(display);
and again
> + }
> + }
> +
> + qmp_expire_password(&opts, &err);
> + g_free(opts.time);
> + g_free(opts.u.vnc.display);
> hmp_handle_error(mon, err);
> }
>
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 5c0d5e116b..cb229c01f8 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
...
> diff --git a/qapi/ui.json b/qapi/ui.json
> index b2cf7a6759..494c92a65e 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -9,22 +9,23 @@
> { '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' } ] }
> +
> ##
> # @set_password:
> #
> # Sets the password of a remote display session.
> #
> -# @protocol: - 'vnc' to modify the VNC server password
> -# - 'spice' to modify the Spice server password
> -#
> -# @password: the new 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
> -#
> # Returns: - Nothing on success
> # - If Spice is not enabled, DeviceNotFound
> #
> @@ -37,33 +38,101 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'set_password',
> - 'data': {'protocol': 'str', 'password': 'str', '*connected': 'str'} }
> +{ 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' }
> +
> +##
> +# @SetPasswordOptions:
> +#
> +# Data required to set a new password on a display server protocol.
> +#
> +# @protocol: - 'vnc' to modify the VNC server password
> +# - 'spice' to modify the Spice server password
> +#
> +# @password: the new password
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'union': 'SetPasswordOptions',
> + 'base': { 'protocol': 'DisplayProtocol',
> + 'password': 'str' },
> + 'discriminator': 'protocol',
> + 'data': { 'vnc': 'SetPasswordOptionsVnc',
> + 'spice': 'SetPasswordOptionsSpice' } }
> +
> +##
> +# @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' ] }
> +
> +##
> +# @SetPasswordActionVnc:
> +#
> +# See @SetPasswordAction. VNC only supports the keep action. 'connection'
> +# should just be omitted for VNC, this is kept for backwards compatibility.
> +#
> +# @keep: maintain existing clients
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'enum': 'SetPasswordActionVnc',
> + 'data': [ 'keep' ] }
> +
> +##
> +# @SetPasswordOptionsSpice:
> +#
> +# Options for set_password specific to the VNC procotol.
> +#
> +# @connected: How to handle existing clients when changing the
> +# password. If nothing is specified, defaults to 'keep'.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'struct': 'SetPasswordOptionsSpice',
> + 'data': { '*connected': 'SetPasswordAction' } }
> +
> +##
> +# @SetPasswordOptionsVnc:
> +#
> +# Options for set_password specific to the VNC procotol.
> +#
> +# @display: The id of the display where the password should be changed.
> +# Defaults to the first.
> +#
> +# @connected: How to handle existing clients when changing the
> +# password. Will always be 'keep' for VNC, parameter is
> +# deprecated and should be omitted.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'struct': 'SetPasswordOptionsVnc',
> + 'data': { '*display': 'str', '*connected': 'SetPasswordActionVnc' } }
Doesn't have to be in this patch (as you are refactoring a
pre-existing problem), but we should consider a followup patch to
actually use the QAPI 'features':'deprected' ability to actually
declare the deprecation through introspection.
Overall, the QAPI changes look sane to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
