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

From: Markus Armbruster <armbru@redhat.com>
To: Fabian Ebner <f.ebner@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>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Thomas Lamprecht" <t.lamprecht@proxmox.com>
Subject: Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Date: Fri, 21 Jan 2022 16:54:02 +0100	[thread overview]
Message-ID: <87y2392ig5.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <17e5677f-b20d-7b48-5946-54297ebf9780@proxmox.com> (Fabian Ebner's message of "Fri, 21 Jan 2022 14:20:50 +0100")

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.

> Or maybe that's a good indication that it's really not worth it ;)?

Possibly.

> P.S. Thank you for the QAPI-related explanation in the other mail!

You're welcome!