* [PATCH 0/2] VNC-related HMP/QMP fixes @ 2021-08-25 9:36 Stefan Reiter 2021-08-25 9:37 ` [PATCH 1/2] monitor/hmp: correctly invert password argument detection again Stefan Reiter 2021-08-25 9:37 ` [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID Stefan Reiter 0 siblings, 2 replies; 9+ messages in thread From: Stefan Reiter @ 2021-08-25 9:36 UTC (permalink / raw) To: Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini, Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht Cc: qemu-devel Since the removal of the generic 'qmp_change' command, one can no longer replace the 'default' VNC display listen address at runtime (AFAIK). For our users who need to set up a secondary VNC access port, this means configuring a second VNC display (in addition to our standard one for web-access), but it turns out one cannot set a password on this second display at the moment, as the 'set_password' call only operates on the 'default' display. Additionally, using secret objects, the password is only read once at startup. This could be considered a bug too, but is not touched in this series and left for a later date. Stefan Reiter (2): monitor/hmp: correctly invert password argument detection again monitor: allow VNC related QMP and HMP commands to take a display ID hmp-commands.hx | 28 +++++++++++++++------------- monitor/hmp-cmds.c | 22 +++++++++++++++++++--- monitor/qmp-cmds.c | 9 +++++---- qapi/ui.json | 12 ++++++++++-- 4 files changed, 49 insertions(+), 22 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] monitor/hmp: correctly invert password argument detection again 2021-08-25 9:36 [PATCH 0/2] VNC-related HMP/QMP fixes Stefan Reiter @ 2021-08-25 9:37 ` Stefan Reiter 2021-08-25 10:42 ` Marc-André Lureau 2021-08-25 9:37 ` [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID Stefan Reiter 1 sibling, 1 reply; 9+ messages in thread From: Stefan Reiter @ 2021-08-25 9:37 UTC (permalink / raw) To: Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini, Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht Cc: qemu-devel Commit cfb5387a1d 'hmp: remove "change vnc TARGET" command' claims to remove the HMP "change vnc" command, but doesn't actually do that. Instead if rewires it to use 'qmp_change_vnc_password', and in the process inverts the argument detection - ignoring the first issue, this inversion is wrong, as this will now ask the user for a password if one is already provided, and simply fail if none is given. Fixes: cfb5387a1d ("hmp: remove "change vnc TARGET" command") Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> --- monitor/hmp-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index f7a211e5a4..31366e6331 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1591,7 +1591,7 @@ void hmp_change(Monitor *mon, const QDict *qdict) } if (strcmp(target, "passwd") == 0 || strcmp(target, "password") == 0) { - if (arg) { + if (!arg) { MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); monitor_read_password(hmp_mon, hmp_change_read_arg, NULL); return; -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] monitor/hmp: correctly invert password argument detection again 2021-08-25 9:37 ` [PATCH 1/2] monitor/hmp: correctly invert password argument detection again Stefan Reiter @ 2021-08-25 10:42 ` Marc-André Lureau 0 siblings, 0 replies; 9+ messages in thread From: Marc-André Lureau @ 2021-08-25 10:42 UTC (permalink / raw) To: Stefan Reiter Cc: Wolfgang Bumiller, QEMU, Dr. David Alan Gilbert, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Eric Blake, Thomas Lamprecht [-- Attachment #1: Type: text/plain, Size: 1457 bytes --] Hi On Wed, Aug 25, 2021 at 1:38 PM Stefan Reiter <s.reiter@proxmox.com> wrote: > Commit cfb5387a1d 'hmp: remove "change vnc TARGET" command' claims to > remove the HMP "change vnc" command, but doesn't actually do that. > Instead if rewires it to use 'qmp_change_vnc_password', and in the > process inverts the argument detection - ignoring the first issue, this > inversion is wrong, as this will now ask the user for a password if one > is already provided, and simply fail if none is given. > > Fixes: cfb5387a1d ("hmp: remove "change vnc TARGET" command") > Signed-off-by: Stefan Reiter <s.reiter@proxmox.com> > Oh.. hmp, hear me please! Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- > monitor/hmp-cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index f7a211e5a4..31366e6331 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1591,7 +1591,7 @@ void hmp_change(Monitor *mon, const QDict *qdict) > } > if (strcmp(target, "passwd") == 0 || > strcmp(target, "password") == 0) { > - if (arg) { > + if (!arg) { > MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, > common); > monitor_read_password(hmp_mon, hmp_change_read_arg, NULL); > return; > -- > 2.30.2 > > > > -- Marc-André Lureau [-- Attachment #2: Type: text/html, Size: 2343 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID 2021-08-25 9:36 [PATCH 0/2] VNC-related HMP/QMP fixes Stefan Reiter 2021-08-25 9:37 ` [PATCH 1/2] monitor/hmp: correctly invert password argument detection again Stefan Reiter @ 2021-08-25 9:37 ` Stefan Reiter 2021-08-25 10:59 ` Marc-André Lureau 1 sibling, 1 reply; 9+ messages in thread From: Stefan Reiter @ 2021-08-25 9:37 UTC (permalink / raw) To: Dr. David Alan Gilbert, Markus Armbruster, Paolo Bonzini, Eric Blake, Gerd Hoffmann, Wolfgang Bumiller, Thomas Lamprecht Cc: qemu-devel 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. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID 2021-08-25 9:37 ` [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID Stefan Reiter @ 2021-08-25 10:59 ` Marc-André Lureau 2021-08-25 11:26 ` Stefan Reiter 2021-08-25 19:16 ` Eric Blake 0 siblings, 2 replies; 9+ messages in thread From: Marc-André Lureau @ 2021-08-25 10:59 UTC (permalink / raw) To: Stefan Reiter Cc: Wolfgang Bumiller, QEMU, Dr. David Alan Gilbert, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Eric Blake, Thomas Lamprecht [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID 2021-08-25 10:59 ` Marc-André Lureau @ 2021-08-25 11:26 ` Stefan Reiter 2021-08-25 19:20 ` Eric Blake 2021-08-25 19:16 ` Eric Blake 1 sibling, 1 reply; 9+ messages in thread From: Stefan Reiter @ 2021-08-25 11:26 UTC (permalink / raw) To: Marc-André Lureau Cc: Wolfgang Bumiller, QEMU, Dr. David Alan Gilbert, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Eric Blake, Thomas Lamprecht On 8/25/21 12:59 PM, Marc-André Lureau wrote: > 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"...). > Indeed, if there is a better way I'd love to use it. One idea I had was making the parameter 'connected' OR 'display', since the former only supports 'keep' for VNC anyway - but that introduces a weird double-meaning again. > Is it really necessary to add support for HMP? > For us it would be, as we provide an easy HMP interface to our users, but not a QMP one, so it ended up being a bit of a regression with 6.0. >> 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 >> >> >> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID 2021-08-25 11:26 ` Stefan Reiter @ 2021-08-25 19:20 ` Eric Blake 0 siblings, 0 replies; 9+ messages in thread From: Eric Blake @ 2021-08-25 19:20 UTC (permalink / raw) To: Stefan Reiter Cc: Wolfgang Bumiller, QEMU, Dr. David Alan Gilbert, Markus Armbruster, Marc-André Lureau, Gerd Hoffmann, Paolo Bonzini, Thomas Lamprecht On Wed, Aug 25, 2021 at 01:26:09PM +0200, Stefan Reiter wrote: > > It's not something done with other commands afaik, feels a bit awkward (the > > "connected = display"...). > > > > Indeed, if there is a better way I'd love to use it. > > One idea I had was making the parameter 'connected' OR 'display', since > the former only supports 'keep' for VNC anyway - but that introduces a > weird double-meaning again. That's not too bad. See for example hmp_hostfwd_add, which merely names its two arguments arg1 and arg2. Naming your argument 'arg' instead of 'connected' or 'display' lets you interpret it in whatever context-sensitive manner is easiest for your use case. > > Is it really necessary to add support for HMP? > > > > For us it would be, as we provide an easy HMP interface to our users, but > not a QMP one, so it ended up being a bit of a regression with 6.0. Okay, you do have a point that until users can reliably use QMP to do the same, then keeping pre-existing HMP commands working is worthwhile. But long-term HMP stability is not an end goal. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID 2021-08-25 10:59 ` Marc-André Lureau 2021-08-25 11:26 ` Stefan Reiter @ 2021-08-25 19:16 ` Eric Blake 2021-08-31 12:20 ` Gerd Hoffmann 1 sibling, 1 reply; 9+ messages in thread From: Eric Blake @ 2021-08-25 19:16 UTC (permalink / raw) To: Marc-André Lureau Cc: Wolfgang Bumiller, QEMU, Stefan Reiter, Dr. David Alan Gilbert, Markus Armbruster, Gerd Hoffmann, Paolo Bonzini, Thomas Lamprecht On Wed, Aug 25, 2021 at 02:59:06PM +0400, Marc-André Lureau wrote: > > 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? What's more, we have explicitly documented that HMP is not stable. We don't need to bend over backwards to keep old HMP command lines working if it is saner to just rearrange the command to our new liking. You could also supply the display via a flag argument (set_password -d vnc2 password) instead of trying to place it in a positional argument. > > +++ 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID 2021-08-25 19:16 ` Eric Blake @ 2021-08-31 12:20 ` Gerd Hoffmann 0 siblings, 0 replies; 9+ messages in thread From: Gerd Hoffmann @ 2021-08-31 12:20 UTC (permalink / raw) To: Eric Blake Cc: Wolfgang Bumiller, QEMU, Stefan Reiter, Dr. David Alan Gilbert, Markus Armbruster, Marc-André Lureau, Paolo Bonzini, Thomas Lamprecht Hi, > You could also supply the display via a flag argument > (set_password -d vnc2 password) instead of trying to place it in a > positional argument. I like that idea. take care, Gerd ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-08-31 12:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-25 9:36 [PATCH 0/2] VNC-related HMP/QMP fixes Stefan Reiter 2021-08-25 9:37 ` [PATCH 1/2] monitor/hmp: correctly invert password argument detection again Stefan Reiter 2021-08-25 10:42 ` Marc-André Lureau 2021-08-25 9:37 ` [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID Stefan Reiter 2021-08-25 10:59 ` Marc-André Lureau 2021-08-25 11:26 ` Stefan Reiter 2021-08-25 19:20 ` Eric Blake 2021-08-25 19:16 ` Eric Blake 2021-08-31 12:20 ` Gerd Hoffmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.