docs/about/deprecated.rst | 6 ++ hmp-commands.hx | 24 +++--- monitor/hmp-cmds.c | 48 +++++++++++- monitor/hmp.c | 19 ++++- monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 54 ++++--------- qapi/ui.json | 156 ++++++++++++++++++++++++++++++++----- 7 files changed, 236 insertions(+), 74 deletions(-)
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. v6 -> v7: * remove g_strdup and g_free, use strings directly * squash in last patch v5 -> v6: * consider feedback from Markus' review, mainly: * fix crash bug in patch 1 (sorry, artifact of patch-splitting) * rely on '!has_param => param == NULL' to shorten code * add note to 'docs/about/deprecated.rst' and touch up comments a bit * go back to g_free instead of qapi_free_* since the latter apparently tries to free the passed in pointer which lives on the stack... * fix bug in HMP parsing (see patch 1) v4 -> v5: * add comment to patch 1 in "monitor-internal.h" * use qapi_free_SetPasswordOptions and friends, don't leak strdups * split QAPI change into 3 seperate patches v3 -> v4: * drop previously patch 1, this was fixed here instead: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html * patch 1: add Eric's R-b * patch 2: remove if-assignment, use 'deprecated' feature in schema v2 -> v3: * refactor QMP schema for set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (4): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate docs/about/deprecated.rst | 6 ++ hmp-commands.hx | 24 +++--- monitor/hmp-cmds.c | 48 +++++++++++- monitor/hmp.c | 19 ++++- monitor/monitor-internal.h | 3 +- monitor/qmp-cmds.c | 54 ++++--------- qapi/ui.json | 156 ++++++++++++++++++++++++++++++++----- 7 files changed, 236 insertions(+), 74 deletions(-) -- 2.30.2
I'm done reviewing. David, care to have another look at the HMP part?
* Markus Armbruster (armbru@redhat.com) wrote: > I'm done reviewing. David, care to have another look at the HMP part? Yep, looking good to me - is that going via qmp, hmp, or vnc ? Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Markus Armbruster (armbru@redhat.com) wrote: >> I'm done reviewing. David, care to have another look at the HMP part? > > Yep, looking good to me - is that going via qmp, hmp, or vnc ? Either is fine with me. David, Gerd, do you have anything queued up already?
On Tue, Oct 26, 2021 at 01:32:29PM +0200, Markus Armbruster wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > * Markus Armbruster (armbru@redhat.com) wrote: > >> I'm done reviewing. David, care to have another look at the HMP part? > > > > Yep, looking good to me - is that going via qmp, hmp, or vnc ? > > Either is fine with me. > > David, Gerd, do you have anything queued up already? Nothing queued atm, no objections to someone else picking this up. Acked-by: Gerd Hoffmann <kraxel@redhat.com> take care, Gerd
Stefan Reiter <s.reiter@proxmox.com> writes: > 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. Queued, thanks!
Markus Armbruster <armbru@redhat.com> writes: > Stefan Reiter <s.reiter@proxmox.com> writes: > >> 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. > > Queued, thanks! Unqueued, because it fails to compile with --disable-vnc and with --disable-spice. I failed to catch this in review, sorry. Making it work takes a tiresome amount of #ifdeffery (sketch appended). Missing: removal of stubs that are no longer used, e.g. vnc_display_password() in ui/vnc-stubs.c. Feels like more trouble than it's worth. To maximize our chances to get this into 6.2, please respin without the 'if' conditionals. Yes, this makes introspection less useful, but it's no worse than before the patch. diff --git a/qapi/ui.json b/qapi/ui.json index 5292617b44..39ca0b5ead 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -69,8 +69,10 @@ 'base': { 'protocol': 'DisplayProtocol', 'password': 'str' }, 'discriminator': 'protocol', - 'data': { 'vnc': 'SetPasswordOptionsVnc', - 'spice': 'SetPasswordOptionsSpice' } } + 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc', + 'if': 'CONFIG_VNC' }, + 'spice': { 'type': 'SetPasswordOptionsSpice', + 'if': 'CONFIG_SPICE' } } } ## # @SetPasswordOptionsSpice: @@ -155,7 +157,8 @@ 'base': { 'protocol': 'DisplayProtocol', 'time': 'str' }, 'discriminator': 'protocol', - 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } } + 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc', + 'if': 'CONFIG_VNC' } } } ## # @ExpirePasswordOptionsVnc: diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index f0f0c82d59..f714b2d316 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1451,24 +1451,40 @@ 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"); +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC) const char *display = qdict_get_try_str(qdict, "display"); +#endif +#ifdef CONFIG_SPICE const char *connected = qdict_get_try_str(qdict, "connected"); +#endif Error *err = NULL; + int proto; SetPasswordOptions opts = { .password = (char *)password, }; - opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, - DISPLAY_PROTOCOL_VNC, &err); + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err); if (err) { goto out; } - if (opts.protocol == DISPLAY_PROTOCOL_VNC) { + switch (proto) { +#ifdef CONFIG_VNC + case -1: + proto = DISPLAY_PROTOCOL_VNC; + /* fall through */ + case DISPLAY_PROTOCOL_VNC: opts.u.vnc.has_display = !!display; opts.u.vnc.display = (char *)display; - } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) { + break; +#else + case -1: + error_setg(&err, "FIXME"); + goto out; +#endif +#ifdef CONFIG_SPICE + case DISPLAY_PROTOCOL_SPICE: opts.u.spice.has_connected = !!connected; opts.u.spice.connected = qapi_enum_parse(&SetPasswordAction_lookup, connected, @@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict) if (err) { goto out; } + break; +#endif + default: + ; } + opts.protocol = proto; qmp_set_password(&opts, &err); out: @@ -1488,22 +1509,34 @@ 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"); +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC) const char *display = qdict_get_try_str(qdict, "display"); +#endif Error *err = NULL; + int proto; ExpirePasswordOptions opts = { .time = (char *)whenstr, }; - opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, - DISPLAY_PROTOCOL_VNC, &err); + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err); if (err) { goto out; } - if (opts.protocol == DISPLAY_PROTOCOL_VNC) { + switch (proto) { +#ifdef CONFIG_VNC + case -1: + proto = DISPLAY_PROTOCOL_VNC; + /* fall through */ + case DISPLAY_PROTOCOL_VNC: opts.u.vnc.has_display = !!display; opts.u.vnc.display = (char *)display; +#else + case -1: + error_setg(&err, "FIXME"); + goto out; +#endif } qmp_expire_password(&opts, &err); diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index 4825d0cbea..69a9c2977a 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp) { int rc = 0; - if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { + switch (opts->protocol) { +#ifdef CONFIG_SPICE + case DISPLAY_PROTOCOL_SPICE: if (!qemu_using_spice(errp)) { return; } rc = qemu_spice.set_passwd(opts->password, opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL, opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT); - } else { - assert(opts->protocol == DISPLAY_PROTOCOL_VNC); + break; +#endif +#ifdef CONFIG_VNC + case DISPLAY_PROTOCOL_VNC: /* Note that setting an empty password will not disable login through * this interface. */ rc = vnc_display_password(opts->u.vnc.display, opts->password); + break; +#endif + default: + abort(); } if (rc != 0) { @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp) when = strtoull(whenstr, NULL, 10); } - if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { + switch (opts->protocol) { +#ifdef CONFIG_SPICE + case DISPLAY_PROTOCOL_SPICE: if (!qemu_using_spice(errp)) { return; } rc = qemu_spice.set_pw_expire(when); - } else { - assert(opts->protocol == DISPLAY_PROTOCOL_VNC); + break; +#endif +#ifdef CONFIG_VNC + case DISPLAY_PROTOCOL_VNC: rc = vnc_display_pw_expire(opts->u.vnc.display, when); + break; +#endif + default: + abort(); } if (rc != 0) {
Am 28.10.21 um 21:37 schrieb Markus Armbruster: > Markus Armbruster <armbru@redhat.com> writes: > >> Stefan Reiter <s.reiter@proxmox.com> writes: >> >>> 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. >> >> Queued, thanks! > > Unqueued, because it fails to compile with --disable-vnc and with > --disable-spice. I failed to catch this in review, sorry. > > Making it work takes a tiresome amount of #ifdeffery (sketch appended). > Missing: removal of stubs that are no longer used, > e.g. vnc_display_password() in ui/vnc-stubs.c. Feels like more trouble > than it's worth. > > To maximize our chances to get this into 6.2, please respin without the > 'if' conditionals. Yes, this makes introspection less useful, but it's > no worse than before the patch. > Unfortunately, Stefan is no longer working with Proxmox, so I would pick up these patches instead. Since the 6.2 ship has long sailed, I suppose the way forward is using the #ifdefs then? From my understanding what should be done is: * In the first patch, fix the typo spotted by Eric Blake and add the R-b tags from this round. * Replace "since 6.2" with "since 7.0" everywhere. * Incorporate the #ifdef handling from below. I had to add another one for the when/whenstr handling in qmp_expire_password to avoid an error with -Werror=unused-but-set-variable. * Add #ifdefs for the unused stubs too? If yes, how to best find them? > > diff --git a/qapi/ui.json b/qapi/ui.json > index 5292617b44..39ca0b5ead 100644 > --- a/qapi/ui.json > +++ b/qapi/ui.json > @@ -69,8 +69,10 @@ > 'base': { 'protocol': 'DisplayProtocol', > 'password': 'str' }, > 'discriminator': 'protocol', > - 'data': { 'vnc': 'SetPasswordOptionsVnc', > - 'spice': 'SetPasswordOptionsSpice' } } > + 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc', > + 'if': 'CONFIG_VNC' }, > + 'spice': { 'type': 'SetPasswordOptionsSpice', > + 'if': 'CONFIG_SPICE' } } } > > ## > # @SetPasswordOptionsSpice: > @@ -155,7 +157,8 @@ > 'base': { 'protocol': 'DisplayProtocol', > 'time': 'str' }, > 'discriminator': 'protocol', > - 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } } > + 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc', > + 'if': 'CONFIG_VNC' } } } > > ## > # @ExpirePasswordOptionsVnc: > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index f0f0c82d59..f714b2d316 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1451,24 +1451,40 @@ 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"); > +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC) > const char *display = qdict_get_try_str(qdict, "display"); > +#endif > +#ifdef CONFIG_SPICE > const char *connected = qdict_get_try_str(qdict, "connected"); > +#endif > Error *err = NULL; > + int proto; > > SetPasswordOptions opts = { > .password = (char *)password, > }; > > - opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, > - DISPLAY_PROTOCOL_VNC, &err); > + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err); > if (err) { > goto out; > } > > - if (opts.protocol == DISPLAY_PROTOCOL_VNC) { > + switch (proto) { > +#ifdef CONFIG_VNC > + case -1: > + proto = DISPLAY_PROTOCOL_VNC; > + /* fall through */ > + case DISPLAY_PROTOCOL_VNC: > opts.u.vnc.has_display = !!display; > opts.u.vnc.display = (char *)display; > - } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) { > + break; > +#else > + case -1: > + error_setg(&err, "FIXME"); > + goto out; > +#endif > +#ifdef CONFIG_SPICE > + case DISPLAY_PROTOCOL_SPICE: > opts.u.spice.has_connected = !!connected; > opts.u.spice.connected = > qapi_enum_parse(&SetPasswordAction_lookup, connected, > @@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict) > if (err) { > goto out; > } > + break; > +#endif > + default: > + ; > } > > + opts.protocol = proto; > qmp_set_password(&opts, &err); > > out: > @@ -1488,22 +1509,34 @@ 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"); > +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC) > const char *display = qdict_get_try_str(qdict, "display"); > +#endif > Error *err = NULL; > + int proto; > > ExpirePasswordOptions opts = { > .time = (char *)whenstr, > }; > > - opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, > - DISPLAY_PROTOCOL_VNC, &err); > + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err); > if (err) { > goto out; > } > > - if (opts.protocol == DISPLAY_PROTOCOL_VNC) { > + switch (proto) { > +#ifdef CONFIG_VNC > + case -1: > + proto = DISPLAY_PROTOCOL_VNC; > + /* fall through */ > + case DISPLAY_PROTOCOL_VNC: > opts.u.vnc.has_display = !!display; > opts.u.vnc.display = (char *)display; > +#else > + case -1: > + error_setg(&err, "FIXME"); > + goto out; > +#endif > } > > qmp_expire_password(&opts, &err); > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > index 4825d0cbea..69a9c2977a 100644 > --- a/monitor/qmp-cmds.c > +++ b/monitor/qmp-cmds.c > @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp) > { > int rc = 0; > > - if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { > + switch (opts->protocol) { > +#ifdef CONFIG_SPICE > + case DISPLAY_PROTOCOL_SPICE: > if (!qemu_using_spice(errp)) { > return; > } > rc = qemu_spice.set_passwd(opts->password, > opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL, > opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT); > - } else { > - assert(opts->protocol == DISPLAY_PROTOCOL_VNC); > + break; > +#endif > +#ifdef CONFIG_VNC > + case DISPLAY_PROTOCOL_VNC: > /* Note that setting an empty password will not disable login through > * this interface. */ > rc = vnc_display_password(opts->u.vnc.display, opts->password); > + break; > +#endif > + default: > + abort(); > } > > if (rc != 0) { > @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp) > when = strtoull(whenstr, NULL, 10); > } > > - if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { > + switch (opts->protocol) { > +#ifdef CONFIG_SPICE > + case DISPLAY_PROTOCOL_SPICE: > if (!qemu_using_spice(errp)) { > return; > } > rc = qemu_spice.set_pw_expire(when); > - } else { > - assert(opts->protocol == DISPLAY_PROTOCOL_VNC); > + break; > +#endif > +#ifdef CONFIG_VNC > + case DISPLAY_PROTOCOL_VNC: > rc = vnc_display_pw_expire(opts->u.vnc.display, when); > + break; > +#endif > + default: > + abort(); > } > > if (rc != 0) { > > >
Fabian Ebner <f.ebner@proxmox.com> writes: > Am 28.10.21 um 21:37 schrieb Markus Armbruster: >> Markus Armbruster <armbru@redhat.com> writes: >> >>> Stefan Reiter <s.reiter@proxmox.com> writes: >>> >>>> 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. >>> >>> Queued, thanks! >> >> Unqueued, because it fails to compile with --disable-vnc and with >> --disable-spice. I failed to catch this in review, sorry. >> >> Making it work takes a tiresome amount of #ifdeffery (sketch appended). >> Missing: removal of stubs that are no longer used, >> e.g. vnc_display_password() in ui/vnc-stubs.c. Feels like more trouble >> than it's worth. >> >> To maximize our chances to get this into 6.2, please respin without the >> 'if' conditionals. Yes, this makes introspection less useful, but it's >> no worse than before the patch. > > Unfortunately, Stefan is no longer working with Proxmox, so I would > pick up these patches instead. Appreciated! > Since the 6.2 ship has long sailed, I suppose the way forward is using > the #ifdefs then? Maybe. We can choose to improve introspection: keep the 'if' conditionals, and fix the C code to compile with --disable-vnc and --disable-spice. Or we can leave it imperfect as it was: drop the 'if' conditionals. If we had a concrete need for better introspection here, the choice would be easy. But as far as I know, we don't. Is better introspection worth the additional work anyway? Since you're volunteering to do the work, you get to decide :) > From my understanding what should be done is: > > * In the first patch, fix the typo spotted by Eric Blake and add the > R-b tags from this round. > > * Replace "since 6.2" with "since 7.0" everywhere. > > * Incorporate the #ifdef handling from below. I had to add another one > for the when/whenstr handling in qmp_expire_password to avoid an > error with -Werror=unused-but-set-variable. > > * Add #ifdefs for the unused stubs too? If yes, how to best find them? For every call you put under #if, check whether there are are any unconditional calls left, and if not, whether there is a stub function for it. If this is too terse, just ask for more help.
Am 28.10.21 um 21:37 schrieb Markus Armbruster: > Markus Armbruster <armbru@redhat.com> writes: > ----8<---- > > diff --git a/qapi/ui.json b/qapi/ui.json > index 5292617b44..39ca0b5ead 100644 > --- a/qapi/ui.json > +++ b/qapi/ui.json > @@ -69,8 +69,10 @@ > 'base': { 'protocol': 'DisplayProtocol', > 'password': 'str' }, > 'discriminator': 'protocol', > - 'data': { 'vnc': 'SetPasswordOptionsVnc', > - 'spice': 'SetPasswordOptionsSpice' } } > + 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc', > + 'if': 'CONFIG_VNC' }, > + 'spice': { 'type': 'SetPasswordOptionsSpice', > + 'if': 'CONFIG_SPICE' } } } > > ## > # @SetPasswordOptionsSpice: > @@ -155,7 +157,8 @@ > 'base': { 'protocol': 'DisplayProtocol', > 'time': 'str' }, > 'discriminator': 'protocol', > - 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } } > + 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc', > + 'if': 'CONFIG_VNC' } } } > So I decided to give the #ifdef approach a go, but if I configure with --disable-spice --disable-vnc, even with the above conditionals, it is still be possible to issue a set_password qmp command, which will then lead to an abort() in the generated code (and the patched qmp_set_password below would do the same if it could be reached): Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"): #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007f4a90d72537 in __GI_abort () at abort.c:79 #2 0x00005583ca03cef3 in visit_type_SetPasswordOptions_members (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71 #3 0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at qapi/qapi-commands-ui.c:49 #4 0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0) at ../qapi/qmp-dispatch.c:129 #5 0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at ../util/async.c:141 Is that expected? Should I add a conditional for {set,expire}_password in the schema too, and add an #if defined(CONFIG_SPICE) || defined(CONFIG_VNC) around the whole {hmp,qmp}_{set,expire}_password functions/declarations in C? Or maybe that's a good indication that it's really not worth it ;)? P.S. Thank you for the QAPI-related explanation in the other mail! ----8<---- > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > index 4825d0cbea..69a9c2977a 100644 > --- a/monitor/qmp-cmds.c > +++ b/monitor/qmp-cmds.c > @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp) > { > int rc = 0; > > - if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { > + switch (opts->protocol) { > +#ifdef CONFIG_SPICE > + case DISPLAY_PROTOCOL_SPICE: > if (!qemu_using_spice(errp)) { > return; > } > rc = qemu_spice.set_passwd(opts->password, > opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL, > opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT); > - } else { > - assert(opts->protocol == DISPLAY_PROTOCOL_VNC); > + break; > +#endif > +#ifdef CONFIG_VNC > + case DISPLAY_PROTOCOL_VNC: > /* Note that setting an empty password will not disable login through > * this interface. */ > rc = vnc_display_password(opts->u.vnc.display, opts->password); > + break; > +#endif > + default: > + abort(); > } > > if (rc != 0) { > @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp) > when = strtoull(whenstr, NULL, 10); > } > > - if (opts->protocol == DISPLAY_PROTOCOL_SPICE) { > + switch (opts->protocol) { > +#ifdef CONFIG_SPICE > + case DISPLAY_PROTOCOL_SPICE: > if (!qemu_using_spice(errp)) { > return; > } > rc = qemu_spice.set_pw_expire(when); > - } else { > - assert(opts->protocol == DISPLAY_PROTOCOL_VNC); > + break; > +#endif > +#ifdef CONFIG_VNC > + case DISPLAY_PROTOCOL_VNC: > rc = vnc_display_pw_expire(opts->u.vnc.display, when); > + break; > +#endif > + default: > + abort(); > } > > if (rc != 0) { > > >
Fabian Ebner <f.ebner@proxmox.com> writes: > Am 28.10.21 um 21:37 schrieb Markus Armbruster: >> Markus Armbruster <armbru@redhat.com> writes: >> > > ----8<---- > >> diff --git a/qapi/ui.json b/qapi/ui.json >> index 5292617b44..39ca0b5ead 100644 >> --- a/qapi/ui.json >> +++ b/qapi/ui.json >> @@ -69,8 +69,10 @@ >> 'base': { 'protocol': 'DisplayProtocol', >> 'password': 'str' }, >> 'discriminator': 'protocol', >> - 'data': { 'vnc': 'SetPasswordOptionsVnc', >> - 'spice': 'SetPasswordOptionsSpice' } } >> + 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc', >> + 'if': 'CONFIG_VNC' }, >> + 'spice': { 'type': 'SetPasswordOptionsSpice', >> + 'if': 'CONFIG_SPICE' } } } >> ## >> # @SetPasswordOptionsSpice: >> @@ -155,7 +157,8 @@ >> 'base': { 'protocol': 'DisplayProtocol', >> 'time': 'str' }, >> 'discriminator': 'protocol', >> - 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } } >> + 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc', >> + 'if': 'CONFIG_VNC' } } } >> > > So I decided to give the #ifdef approach a go, but if I configure with > --disable-spice --disable-vnc, even with the above conditionals, it is > still be possible to issue a set_password qmp command, which will then > lead to an abort() in the generated code (and the patched > qmp_set_password below would do the same if it could be reached): > > Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"): > #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 > #1 0x00007f4a90d72537 in __GI_abort () at abort.c:79 > #2 0x00005583ca03cef3 in visit_type_SetPasswordOptions_members > (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, > errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71 > #3 0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized > out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at > qapi/qapi-commands-ui.c:49 > #4 0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0) > at ../qapi/qmp-dispatch.c:129 > #5 0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at > ../util/async.c:141 > > Is that expected? Should I add a conditional for {set,expire}_password > in the schema too, and add an > #if defined(CONFIG_SPICE) || defined(CONFIG_VNC) > around the whole {hmp,qmp}_{set,expire}_password > functions/declarations in C? I can have a closer look. To make it easy, tell me how I can pull your patches, or, if that's inconvenient for you, send them to me. > Or maybe that's a good indication that it's really not worth it ;)? Possibly. > P.S. Thank you for the QAPI-related explanation in the other mail! You're welcome!
Markus Armbruster <armbru@redhat.com> writes: > Fabian Ebner <f.ebner@proxmox.com> writes: > >> Am 28.10.21 um 21:37 schrieb Markus Armbruster: >>> Markus Armbruster <armbru@redhat.com> writes: >>> >> >> ----8<---- >> >>> diff --git a/qapi/ui.json b/qapi/ui.json >>> index 5292617b44..39ca0b5ead 100644 >>> --- a/qapi/ui.json >>> +++ b/qapi/ui.json >>> @@ -69,8 +69,10 @@ >>> 'base': { 'protocol': 'DisplayProtocol', >>> 'password': 'str' }, >>> 'discriminator': 'protocol', >>> - 'data': { 'vnc': 'SetPasswordOptionsVnc', >>> - 'spice': 'SetPasswordOptionsSpice' } } >>> + 'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc', >>> + 'if': 'CONFIG_VNC' }, >>> + 'spice': { 'type': 'SetPasswordOptionsSpice', >>> + 'if': 'CONFIG_SPICE' } } } >>> ## >>> # @SetPasswordOptionsSpice: >>> @@ -155,7 +157,8 @@ >>> 'base': { 'protocol': 'DisplayProtocol', >>> 'time': 'str' }, >>> 'discriminator': 'protocol', >>> - 'data': { 'vnc': 'ExpirePasswordOptionsVnc' } } >>> + 'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc', >>> + 'if': 'CONFIG_VNC' } } } >>> >> >> So I decided to give the #ifdef approach a go, but if I configure with >> --disable-spice --disable-vnc, even with the above conditionals, it is >> still be possible to issue a set_password qmp command, which will then >> lead to an abort() in the generated code (and the patched >> qmp_set_password below would do the same if it could be reached): >> >> Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"): >> #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 >> #1 0x00007f4a90d72537 in __GI_abort () at abort.c:79 >> #2 0x00005583ca03cef3 in visit_type_SetPasswordOptions_members >> (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, >> errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71 >> #3 0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized >> out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at >> qapi/qapi-commands-ui.c:49 >> #4 0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0) >> at ../qapi/qmp-dispatch.c:129 >> #5 0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at >> ../util/async.c:141 >> >> Is that expected? Should I add a conditional for {set,expire}_password >> in the schema too, and add an >> #if defined(CONFIG_SPICE) || defined(CONFIG_VNC) >> around the whole {hmp,qmp}_{set,expire}_password >> functions/declarations in C? > > I can have a closer look. To make it easy, tell me how I can pull your > patches, or, if that's inconvenient for you, send them to me. I got them by e-mail, thanks! The dealloc visitor for unions (here: SetPasswordOptions) falls apart when the tag enum (here: DisplayProtocol) is effectively empty. The dealloc visitor's job is to recursively free a QAPI object. Unlike the other visitors, the dealloc visitor needs to work on half-constructed objects where parts are still zero. Easy, because freeing null pointers does nothing. Complication: for a union, the common visitor core still needs to decide which branch to enter. If we never constructed the union's tag value, it's zero, and so is the branch corresponding to the zero tag value. The visitor core happily goes down that branch, and the dealloc visitor happily does nothing for it. Not exactly the cleanest solution ever, but it works. *Except* when the tag enum is empty. Then we run into the abort() here: bool visit_type_SetPasswordOptions_members(Visitor *v, SetPasswordOptions *obj, Error **errp) { if (!visit_type_q_obj_SetPasswordOptions_base_members(v, (q_obj_SetPasswordOptions_base *)obj, errp)) { return false; } switch (obj->protocol) { #if defined(CONFIG_VNC) case DISPLAY_PROTOCOL_VNC: return visit_type_SetPasswordOptionsVnc_members(v, &obj->u.vnc, errp); #endif /* defined(CONFIG_VNC) */ #if defined(CONFIG_SPICE) case DISPLAY_PROTOCOL_SPICE: return visit_type_SetPasswordOptionsSpice_members(v, &obj->u.spice, errp); #endif /* defined(CONFIG_SPICE) */ default: abort(); } return true; } This is actually why the QAPI generator rejects a union with an empty tag enum (test case tests/qapi-schema/union-empty.json): it's a restriction to protect the dealloc visitor. The QAPI generator doesn't reject a union with a tag enum where all members are conditional. Hole in the restriction. We can either plug the hole in the restriction, or lift the restriction. >> Or maybe that's a good indication that it's really not worth it ;)? > > Possibly. Let's drop the 'if' conditionals in the interest of decoupling this series from the QAPI infrastructure fixes needed to make them work. But first see my question about display-reload upthread. >> P.S. Thank you for the QAPI-related explanation in the other mail! > > You're welcome!
Stefan Reiter <s.reiter@proxmox.com> writes: > 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. Related: Vladimir recently posted a patch to add a new command for changing VNC server listening addresses. Daniel asked him to work it into display-reload instead[1]. Vladimir complied[2]. Daniel, what do you think about this one? Should it also use display-reload? [1] Message-ID: <YdRJ06CS4qoDJI/F@redhat.com> https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00292.html [2] Message-ID: <20220118160909.2502374-1-vsementsov@virtuozzo.com> https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg03864.html
On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote: > Stefan Reiter <s.reiter@proxmox.com> writes: > > > 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. > > Related: Vladimir recently posted a patch to add a new command for > changing VNC server listening addresses. Daniel asked him to work it > into display-reload instead[1]. Vladimir complied[2]. > > Daniel, what do you think about this one? Should it also use > display-reload? I'd ultimately intend to deprecate & remove the direct setting of passwords on the CLI, and exclusively rely on the 'secret' object for passing in passwords. With this in mind, I'd not be enthusiastic about adding new commands for changing passwords in QMP directly, rather I think we should have a way to change the 'secret' object in use. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé: > On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote: >> Stefan Reiter <s.reiter@proxmox.com> writes: >> >>> 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. >> >> Related: Vladimir recently posted a patch to add a new command for >> changing VNC server listening addresses. Daniel asked him to work it >> into display-reload instead[1]. Vladimir complied[2]. >> >> Daniel, what do you think about this one? Should it also use >> display-reload? > > I'd ultimately intend to deprecate & remove the direct setting of > passwords on the CLI, and exclusively rely on the 'secret' object > for passing in passwords. With this in mind, I'd not be enthusiastic > about adding new commands for changing passwords in QMP directly, > rather I think we should have a way to change the 'secret' object > in use. > How should I proceed with this series then? Does adding the new argument for the display ID count as "adding new commands"? If what I should do is switching to only using secret objects, would the plan be something like the following? 1. Add an option to display-reload for switching the display's password-secret while adding SPICE as a valid display type. 2. Also include the set password action (i.e. disconnect/fail/keep) and expiration time as part of that option. 3. Extend display-reload to also take an optional display ID for VNC. 4. Deprecate expire_password and set_password. > Regards, > Daniel
On Mon, Jan 31, 2022 at 10:45:08AM +0100, Fabian Ebner wrote: > Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé: > > On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote: > >> Stefan Reiter <s.reiter@proxmox.com> writes: > >> > >>> 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. > >> > >> Related: Vladimir recently posted a patch to add a new command for > >> changing VNC server listening addresses. Daniel asked him to work it > >> into display-reload instead[1]. Vladimir complied[2]. > >> > >> Daniel, what do you think about this one? Should it also use > >> display-reload? > > > > I'd ultimately intend to deprecate & remove the direct setting of > > passwords on the CLI, and exclusively rely on the 'secret' object > > for passing in passwords. With this in mind, I'd not be enthusiastic > > about adding new commands for changing passwords in QMP directly, > > rather I think we should have a way to change the 'secret' object > > in use. > > > > How should I proceed with this series then? Does adding the new argument > for the display ID count as "adding new commands"? Ok, so I've gone back and properly read the series. Since you're simply extending existing commands with new arguments I've no objection to carrying on with this approach. We should still aim to have a general purpose command for live config changes, but that shouldn't be considered a blocker for you series here, as your series isn't making the existing situation worse. > If what I should do is switching to only using secret objects, would the > plan be something like the following? > > 1. Add an option to display-reload for switching the display's > password-secret while adding SPICE as a valid display type. > 2. Also include the set password action (i.e. disconnect/fail/keep) and > expiration time as part of that option. > 3. Extend display-reload to also take an optional display ID for VNC. > 4. Deprecate expire_password and set_password. I've realized that we shouldn't overload display-reload for dual purposes. We have two distinct usage scenarios that are meaningul - Re-loading the value of the existing secret - Changing which secret is used The 'display-reload' command is reasonable for the first. We should have a 'display-update' command for the second. Either way, I don't think this series should be blocked on this since it is merely modifying an existing command. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.