Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password

[Markus Armbruster]

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);
>>>   }
>>>   
>> 
>> [...]