[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_pass
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password |
Date: |
Thu, 21 Oct 2021 11:35:37 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
Stefan Reiter <s.reiter@proxmox.com> writes:
> On 10/21/21 7:05 AM, Markus Armbruster wrote:
>> Stefan Reiter <s.reiter@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 <armbru@redhat.com>
>>> Signed-off-by: Stefan Reiter <s.reiter@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);
>>> }
>>>
>>
>> [...]
- [PATCH v6 4/5] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate, (continued)
- [PATCH v6 4/5] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate, Stefan Reiter, 2021/10/20
- [PATCH v6 5/5] docs: add deprecation note about 'set_password' param 'connected', Stefan Reiter, 2021/10/20
- [PATCH v6 1/5] monitor/hmp: add support for flag argument with value, Stefan Reiter, 2021/10/20
- [PATCH v6 2/5] qapi/monitor: refactor set/expire_password with enums, Stefan Reiter, 2021/10/20
- [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password, Stefan Reiter, 2021/10/20