Re: [PATCH 2/2] qapi/ui: introduce change-vnc-listen

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

21.12.2021 17:15, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Add command that can change addresses where VNC server listens for new
>> connections. Prior to 6.0 this functionality was available through
>> 'change' qmp command which was deleted.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/about/removed-features.rst |  3 ++-
>>   qapi/ui.json                    | 12 ++++++++++++
>>   ui/vnc.c                        | 26 ++++++++++++++++++++++++++
>>   3 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
>> index d42c3341de..20e6901a82 100644
>> --- a/docs/about/removed-features.rst
>> +++ b/docs/about/removed-features.rst
>> @@ -348,7 +348,8 @@ documentation of ``query-hotpluggable-cpus`` for additional details.
>>   ``change`` (removed in 6.0)
>>   '''''''''''''''''''''''''''
>>   
>> -Use ``blockdev-change-medium`` or ``change-vnc-password`` instead.
>> +Use ``blockdev-change-medium`` or ``change-vnc-password`` or
>> +``change-vnc-listen`` instead.
>>   
>>   ``query-events`` (removed in 6.0)
>>   '''''''''''''''''''''''''''''''''
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index d7567ac866..14e6fe0b4c 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -1304,3 +1304,15 @@
>>   { 'command': 'display-reload',
>>     'data': 'DisplayReloadOptions',
>>     'boxed' : true }
>> +
>> +##
>> +# @change-vnc-listen:
>> +#
>> +# Change set of addresses to listen for connections.
> 
> Please document the arguments:
> 
>     # @id: lorem ipsum
>     #
>     # @address: dolor sit amet
>     #
>     # @websockets: consectetur adipisici elit

Oops :)

# @id: vnc display identifier
#
# @addresses: list of addresses for listen at
#
# @websockets: list of addresses to listen with websockets

> 
>> +#
>> +# Since: 7.0
>> +#
>> +##
>> +{ 'command': 'change-vnc-listen',
>> +  'data': { 'id': 'str', 'addresses': ['SocketAddress'],
>> +            '*websockets': ['SocketAddress'] } }
> 
> Lacks 'if': 'CONFIG_VNC'.

Oops, yes.

> 
> We already have change-vnc-password.  You add change-vnc-listen.  Is
> there anything else we might want to change?

I don't know. I have a request to change only the port of connection.

But creating a special command to change only the port is too specific.

On the other hand, creating command that will allow to change many other vnc parameters means deeper refactoring the vnc code, it's too much for me.

Old removed "change" command allowed to change many vnc arguments as I understand, but they we parsed from one string argument, which is bad for QMP.

So, I decided that the golden mean is make an interface to change the addresses to listen.

Actually, I don't need "websockets", and even don't know how to test them, so we can drop this parameter for now, it's simple to add it later on demand. Or we can keep it as is.

> 
> Aside: what's the difference between change-vnc-password and
> set_password?

Looking at code, the difference is that set_password can also change password on spice, and has some additional logic  with "connected" argument.

> 
> [...]
> 


-- 
Best regards,
Vladimir