Stefan Reiter <s.rei...@proxmox.com> writes: > On 10/21/21 7:05 AM, Markus Armbruster wrote: >> Stefan Reiter <s.rei...@proxmox.com> writes: >> >>> It is possible to specify more than one VNC server on the command line, >>> either with an explicit ID or the auto-generated ones à la "default", >>> "vnc2", "vnc3", ... >>> >>> It is not possible to change the password on one of these extra VNC >>> displays though. Fix this by adding a "display" parameter to the >>> "set_password" and "expire_password" QMP and HMP commands. >>> >>> For HMP, the display is specified using the "-d" value flag. >>> >>> For QMP, the schema is updated to explicitly express the supported >>> variants of the commands with protocol-discriminated unions. >>> >>> Suggested-by: Markus Armbruster <arm...@redhat.com> >>> Signed-off-by: Stefan Reiter <s.rei...@proxmox.com> >>> ---
[...] >>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c >>> index b8abe69609..daa4a8e106 100644 >>> --- a/monitor/hmp-cmds.c >>> +++ b/monitor/hmp-cmds.c >>> @@ -1451,26 +1451,39 @@ void hmp_set_password(Monitor *mon, const QDict >>> *qdict) >>> { >>> const char *protocol = qdict_get_str(qdict, "protocol"); >>> const char *password = qdict_get_str(qdict, "password"); >>> + const char *display = qdict_get_try_str(qdict, "display"); >>> const char *connected = qdict_get_try_str(qdict, "connected"); >>> Error *err = NULL; >>> - DisplayProtocol proto; >>> - SetPasswordAction conn; >>> >>> - proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, >>> - DISPLAY_PROTOCOL_VNC, &err); >>> + SetPasswordOptions opts = { >>> + .password = g_strdup(password), >>> + .u.vnc.display = NULL, >>> + }; >>> + >>> + opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, >>> + DISPLAY_PROTOCOL_VNC, &err); >>> if (err) { >>> goto out; >>> } >>> >>> - conn = qapi_enum_parse(&SetPasswordAction_lookup, connected, >>> - SET_PASSWORD_ACTION_KEEP, &err); >>> - if (err) { >>> - goto out; >>> + if (opts.protocol == DISPLAY_PROTOCOL_VNC) { >>> + opts.u.vnc.has_display = !!display; >>> + opts.u.vnc.display = g_strdup(display); >>> + } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) { >>> + opts.u.spice.has_connected = !!connected; >>> + opts.u.spice.connected = >>> + qapi_enum_parse(&SetPasswordAction_lookup, connected, >>> + SET_PASSWORD_ACTION_KEEP, &err); >>> + if (err) { >>> + goto out; >>> + } >>> } >>> >>> - qmp_set_password(proto, password, !!connected, conn, &err); >>> + qmp_set_password(&opts, &err); >>> >>> out: >>> + g_free(opts.password); >>> + g_free(opts.u.vnc.display); >> >> Uh-oh... >> >> For DISPLAY_PROTOCOL_SPICE, we execute >> >> .u.vnc.display = NULL, >> opts.u.spice.has_connected = !!connected; >> opts.u.spice.connected = >> qapi_enum_parse(&SetPasswordAction_lookup, connected, >> SET_PASSWORD_ACTION_KEEP, &err); >> opts.u.vnc.has_display = !!display; >> g_free(opts.u.vnc.display); >> >> The assignments to opts.u.spice.has_connected and opts.u.spice_connected >> clobber opts.u.vnc.display. >> >> The simplest fix is to pass @display directly. Precedence: >> hmp_drive_mirror(). > > With "directly", I assume you mean without g_strdup, so: > > opts.u.vnc.display = (char *)display; > > right? Right. > Does it matter that we drop the 'const'? It's ugly, but harmless. qdict_get_try_str() returns const char * to discourage you from messing with the string while it's in the QDict. qmp_set_password() does not actually mess with its argument. >> Do the same for @password, of course. >> >>> hmp_handle_error(mon, err); >>> } >>> >>> @@ -1478,18 +1491,30 @@ void hmp_expire_password(Monitor *mon, const QDict >>> *qdict) >>> { >>> const char *protocol = qdict_get_str(qdict, "protocol"); >>> const char *whenstr = qdict_get_str(qdict, "time"); >>> + const char *display = qdict_get_try_str(qdict, "display"); >>> Error *err = NULL; >>> - DisplayProtocol proto; >>> >>> - proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, >>> - DISPLAY_PROTOCOL_VNC, &err); >>> + ExpirePasswordOptions opts = { >>> + .time = g_strdup(whenstr), >>> + .u.vnc.display = NULL, >>> + }; >>> + >>> + opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol, >>> + DISPLAY_PROTOCOL_VNC, &err); >>> if (err) { >>> goto out; >>> } >>> >>> - qmp_expire_password(proto, whenstr, &err); >>> + if (opts.protocol == DISPLAY_PROTOCOL_VNC) { >>> + opts.u.vnc.has_display = !!display; >>> + opts.u.vnc.display = g_strdup(display); >>> + } >> >> Use of -d with spice are silently ignored. Do we care? Same for >> hmp_set_password() above. > > Depends on you, I don't. I think it's not worth catching even more > in HMP, since it's clear there's only one SPICE display anyway, and > it's all documented. Up to the HMP maintainer, and we'll take silence as tacit agreement with you :) >>> + >>> + qmp_expire_password(&opts, &err); >>> >>> out: >>> + g_free(opts.time); >>> + g_free(opts.u.vnc.display); >> >> No uh-oh here, since opts.u.vnc is actually the only member of opts.u. >> Still, let's pass @time and @display directly for consistency and >> robustness. >> >>> hmp_handle_error(mon, err); >>> } >>> >> >> [...]