| Message ID | 20210901151748.1712048-4-s.reiter@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VNC-related HMP/QMP fixes | expand |
On Wed, Sep 01, 2021 at 05:17:48PM +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. > > Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> > --- QMP review: > +++ 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', Pre-existing, but given the documentation that protocol is either 'vnc' or 'spice', this feels like set_password should take a discriminated union type with 'protocol' as an enum type,... > + '*display': 'str'} } ...so that you only add the optional 'display' member to 'vnc'. This would keep the status quo of rejecting it as invalid when protocol is 'spice', and make it easier to introspect that no other protocols are supported. Markus may have better advice on whether cleaning this up is worth it. > > ## > # @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'} } This would benefit from the same treatment, if we decide to use a QAPI enum type and discriminated union.
Eric Blake <eblake@redhat.com> writes: > On Wed, Sep 01, 2021 at 05:17:48PM +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. >> >> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> >> --- > > QMP review: > >> +++ 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', > > Pre-existing, but given the documentation that protocol is either > 'vnc' or 'spice', this feels like set_password should take a > discriminated union type with 'protocol' as an enum type,... > >> + '*display': 'str'} } > > ...so that you only add the optional 'display' member to 'vnc'. This > would keep the status quo of rejecting it as invalid when protocol is > 'spice', and make it easier to introspect that no other protocols are > supported. > > Markus may have better advice on whether cleaning this up is worth it. Changing @protocol from str to enum is straightforward, and backward compatible. qmp_set_password() becomes simpler (we lose a failure mode). If we ever add another protocol, introspection will show it. It also reflects CONFIG_VNC and CONFIG_SPICE, which is perhaps less useful than it was before modularization, but still nice. Yes, please. Same for @connected. We may have more 'str' parameters that should be enum elsewhere. I'm not demanding you hunt them down :) Adding the new parameter only to the protocol that actually supports it is more complicated. Untested: { 'command': 'set_password', 'boxed': true, 'data': 'SetPasswordOptions' } { 'union': 'SetPasswordOptions', 'base': { 'protocol: 'PasswordProtocol', 'connected': 'FailDisconnectKeep' }, 'discriminator': protocol', 'data': { 'vnc': 'SetPasswordOptionsVnc' } } { 'enum': 'PasswordProtocol' 'data': [ { 'name': 'vnc', 'if': 'CONFIG_VNC' }, { 'name': 'spice', 'if': 'CONFIG_SPICE } ] } { 'enum': 'FailDisconnectKeep', 'data': [ 'fail', 'disconnect', 'keep' ] } { 'struct': 'SetPasswordOptionsVnc', 'data': { '*display': 'str } } Advangages are similar: qmp_set_password() doesn't have to reject @display for protocols other than 'vnc', and introspection is more accurate. Please give it a try. >> >> ## >> # @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'} } > > This would benefit from the same treatment, if we decide to use a QAPI > enum type and discriminated union. Either both or neither.
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]", .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 - 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. +``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:-dS", + .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 a7e197a90b..168ca62371 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1451,10 +1451,12 @@ 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); + qmp_set_password(protocol, password, !!connected, connected, !!display, + display, &err); hmp_handle_error(mon, err); } @@ -1462,9 +1464,10 @@ 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); + 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 5c0d5e116b..b53869d10c 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -164,7 +164,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; @@ -197,7 +198,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'"); @@ -210,7 +211,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; @@ -231,7 +232,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 b2cf7a6759..fa84df9a70 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:
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. Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> --- hmp-commands.hx | 29 +++++++++++++++-------------- monitor/hmp-cmds.c | 7 +++++-- monitor/qmp-cmds.c | 9 +++++---- qapi/ui.json | 12 ++++++++++-- 4 files changed, 35 insertions(+), 22 deletions(-)