| Message ID | 20211220154418.1554279-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/ui: add change-vnc-listen | expand |
Hi On Mon, Dec 20, 2021 at 10:24 PM Vladimir Sementsov-Ogievskiy < vsementsov@virtuozzo.com> wrote: > 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> > Looks good to me, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Could you write an avocado test for it? (tests/avocado/vnc.py) --- > 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. > +# > +# Since: 7.0 > +# > +## > +{ 'command': 'change-vnc-listen', > + 'data': { 'id': 'str', 'addresses': ['SocketAddress'], > + '*websockets': ['SocketAddress'] } } > diff --git a/ui/vnc.c b/ui/vnc.c > index c9e26c70df..69bbf3b6f6 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -4212,6 +4212,32 @@ fail: > vnc_display_close(vd); > } > > +void qmp_change_vnc_listen(const char *id, SocketAddressList *addresses, > + bool has_websockets, SocketAddressList > *websockets, > + Error **errp) > +{ > + VncDisplay *vd = vnc_display_find(id); > + > + if (!vd) { > + error_setg(errp, "VNC display '%s' not active", id); > + return; > + } > + > + if (vd->listener) { > + qio_net_listener_disconnect(vd->listener); > + object_unref(OBJECT(vd->listener)); > + } > + vd->listener = NULL; > + > + if (vd->wslistener) { > + qio_net_listener_disconnect(vd->wslistener); > + object_unref(OBJECT(vd->wslistener)); > + } > + vd->wslistener = NULL; > + > + vnc_display_listen(vd, addresses, websockets, errp); > +} > + > void vnc_display_add_client(const char *id, int csock, bool skipauth) > { > VncDisplay *vd = vnc_display_find(id); > -- > 2.31.1 > > >
21.12.2021 11:13, Marc-André Lureau wrote: > Hi > > On Mon, Dec 20, 2021 at 10:24 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>> wrote: > > 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 <mailto:vsementsov@virtuozzo.com>> > > > Looks good to me, > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>> > > Could you write an avocado test for it? (tests/avocado/vnc.py) Thanks a lot for reviewing! I will.
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 > +# > +# Since: 7.0 > +# > +## > +{ 'command': 'change-vnc-listen', > + 'data': { 'id': 'str', 'addresses': ['SocketAddress'], > + '*websockets': ['SocketAddress'] } } Lacks 'if': 'CONFIG_VNC'. We already have change-vnc-password. You add change-vnc-listen. Is there anything else we might want to change? Aside: what's the difference between change-vnc-password and set_password? [...]
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. > > [...] >
On Mon, Dec 20, 2021 at 04:44:18PM +0100, Vladimir Sementsov-Ogievskiy wrote: > 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. > +# > +# Since: 7.0 > +# > +## > +{ 'command': 'change-vnc-listen', > + 'data': { 'id': 'str', 'addresses': ['SocketAddress'], > + '*websockets': ['SocketAddress'] } } We already have a general purpose command above 'display-reload' for doing live changes to the display backends. THis should instead be { 'struct': 'DisplayReloadOptionsVNC', 'data': { '*tls-certs': 'bool', '*addresses': ['SocketAddress'], '*websockets': ['SocketAddress'] } } if 'addresses' is non-null then the listener can be updated. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Dec 20, 2021 at 04:44:18PM +0100, Vladimir Sementsov-Ogievskiy wrote: >> 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. >> +# >> +# Since: 7.0 >> +# >> +## >> +{ 'command': 'change-vnc-listen', >> + 'data': { 'id': 'str', 'addresses': ['SocketAddress'], >> + '*websockets': ['SocketAddress'] } } > > We already have a general purpose command above 'display-reload' > for doing live changes to the display backends. > > THis should instead be > > { 'struct': 'DisplayReloadOptionsVNC', > 'data': { '*tls-certs': 'bool', > '*addresses': ['SocketAddress'], > '*websockets': ['SocketAddress'] } } > > if 'addresses' is non-null then the listener can be updated. Good point. Gerd, what do you think?
On Thu, Jan 13, 2022 at 05:27:08PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Mon, Dec 20, 2021 at 04:44:18PM +0100, Vladimir Sementsov-Ogievskiy wrote: > >> 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. > >> +# > >> +# Since: 7.0 > >> +# > >> +## > >> +{ 'command': 'change-vnc-listen', > >> + 'data': { 'id': 'str', 'addresses': ['SocketAddress'], > >> + '*websockets': ['SocketAddress'] } } > > > > We already have a general purpose command above 'display-reload' > > for doing live changes to the display backends. > > > > THis should instead be > > > > { 'struct': 'DisplayReloadOptionsVNC', > > 'data': { '*tls-certs': 'bool', > > '*addresses': ['SocketAddress'], > > '*websockets': ['SocketAddress'] } } > > > > if 'addresses' is non-null then the listener can be updated. > > Good point. Gerd, what do you think? I guess you could argue that 'display-reload' is more about reloading state, rather than changing configuration. If we want to make such a distinction, then we could have a very similar 'display-update' command for configuration changes. Regards, Daniel
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. +# +# Since: 7.0 +# +## +{ 'command': 'change-vnc-listen', + 'data': { 'id': 'str', 'addresses': ['SocketAddress'], + '*websockets': ['SocketAddress'] } } diff --git a/ui/vnc.c b/ui/vnc.c index c9e26c70df..69bbf3b6f6 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -4212,6 +4212,32 @@ fail: vnc_display_close(vd); } +void qmp_change_vnc_listen(const char *id, SocketAddressList *addresses, + bool has_websockets, SocketAddressList *websockets, + Error **errp) +{ + VncDisplay *vd = vnc_display_find(id); + + if (!vd) { + error_setg(errp, "VNC display '%s' not active", id); + return; + } + + if (vd->listener) { + qio_net_listener_disconnect(vd->listener); + object_unref(OBJECT(vd->listener)); + } + vd->listener = NULL; + + if (vd->wslistener) { + qio_net_listener_disconnect(vd->wslistener); + object_unref(OBJECT(vd->wslistener)); + } + vd->wslistener = NULL; + + vnc_display_listen(vd, addresses, websockets, errp); +} + void vnc_display_add_client(const char *id, int csock, bool skipauth) { VncDisplay *vd = vnc_display_find(id);
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(-)