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

[Markus Armbruster]

Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>>
>>>> Since the removal of the generic 'qmp_change' command, one can no longer 
>>>> replace
>>>> the 'default' VNC display listen address at runtime (AFAIK). For our users 
>>>> who
>>>> need to set up a secondary VNC access port, this means configuring a 
>>>> second VNC
>>>> display (in addition to our standard one for web-access), but it turns out 
>>>> one
>>>> cannot set a password on this second display at the moment, as the
>>>> 'set_password' call only operates on the 'default' display.
>>>>
>>>> Additionally, using secret objects, the password is only read once at 
>>>> startup.
>>>> This could be considered a bug too, but is not touched in this series and 
>>>> left
>>>> for a later date.
>>>
>>> Queued, thanks!
>> 
>> Unqueued, because it fails to compile with --disable-vnc and with
>> --disable-spice.  I failed to catch this in review, sorry.
>>
>> Making it work takes a tiresome amount of #ifdeffery (sketch appended).
>> Missing: removal of stubs that are no longer used,
>> e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
>> than it's worth.
>> 
>> To maximize our chances to get this into 6.2, please respin without the
>> 'if' conditionals.  Yes, this makes introspection less useful, but it's
>> no worse than before the patch.
>
> Unfortunately, Stefan is no longer working with Proxmox, so I would
> pick up these patches instead.

Appreciated!

> Since the 6.2 ship has long sailed, I suppose the way forward is using
> the #ifdefs then?

Maybe.

We can choose to improve introspection: keep the 'if' conditionals, and
fix the C code to compile with --disable-vnc and --disable-spice.

Or we can leave it imperfect as it was: drop the 'if' conditionals.

If we had a concrete need for better introspection here, the choice
would be easy.  But as far as I know, we don't.  Is better introspection
worth the additional work anyway?  Since you're volunteering to do the
work, you get to decide :)

> From my understanding what should be done is:
>
> * In the first patch, fix the typo spotted by Eric Blake and add the
>   R-b tags from this round.
>
> * Replace "since 6.2" with "since 7.0" everywhere.
>
> * Incorporate the #ifdef handling from below. I had to add another one
>   for the when/whenstr handling in qmp_expire_password to avoid an
>  error with -Werror=unused-but-set-variable.
>
> * Add #ifdefs for the unused stubs too? If yes, how to best find them?

For every call you put under #if, check whether there are are any
unconditional calls left, and if not, whether there is a stub function
for it.  If this is too terse, just ask for more help.