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

From: Markus Armbruster <armbru@redhat.com>
To: Stefan Reiter <s.reiter@proxmox.com>
Cc: "Wolfgang Bumiller" <w.bumiller@proxmox.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	qemu-devel@nongnu.org,
	"Marc-André Lureau" <marcandre.lureau@gmail.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Thomas Lamprecht" <t.lamprecht@proxmox.com>
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	[thread overview]
Message-ID: <87ee8een4m.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <f562acca-2c34-cce5-f918-f94f7962b33e@proxmox.com> (Stefan Reiter's message of "Thu, 21 Oct 2021 10:42:31 +0200")

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