Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes

[Markus Armbruster <armbru@redhat.com>]

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!