| Message ID | 20220118160909.2502374-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi/ui: change vnc addresses | expand |
On Tue, Jan 18, 2022 at 05:09:08PM +0100, Vladimir Sementsov-Ogievskiy wrote: > Add possibility to 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 | 6 +++++- > include/ui/console.h | 2 +- > monitor/qmp-cmds.c | 4 +--- > ui/vnc.c | 37 ++++++++++++++++++++++++++------- > 5 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst > index 4c4da20d0f..b92626a74e 100644 > --- a/docs/about/removed-features.rst > +++ b/docs/about/removed-features.rst > @@ -355,7 +355,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 > +``display-reload`` instead. > > ``query-events`` (removed in 6.0) > ''''''''''''''''''''''''''''''''' > diff --git a/qapi/ui.json b/qapi/ui.json > index 9354f4c467..4c4448f378 100644 > --- a/qapi/ui.json > +++ b/qapi/ui.json > @@ -1293,12 +1293,16 @@ > # Specify the VNC reload options. > # > # @tls-certs: reload tls certs or not. > +# @addresses: If specified, change set of addresses > +# to listen for connections. Addresses configured > +# for websockets are not touched. (since 7.0) > # > # Since: 6.0 > # > ## > { 'struct': 'DisplayReloadOptionsVNC', > - 'data': { '*tls-certs': 'bool' } } > + 'data': { '*tls-certs': 'bool', > + '*addresses': ['SocketAddress'] } } So I'm having second thoughts on this, because I think we in fact need to distinguish between reloads of state related to existing configuration vs applying changes to the actual configuration. For example, when I think about the 'tls-certs' option here, it lets us reload the existing TLS credentials from disk. ie it lets the user re-write the TLS file content on disk and then tell QEMU to reload the files. An equally valuable option would be to tell QEMU to simply use a completely different set of TLS files on disk, rather than re-writing in place. We don't have this feature now, but I think we should anticipate it in the design. So I'm going to suggest that instead of 'display-reload', we should have a general purpose 'display-update' command for modifying existing config and , leave 'display-reload' purely for re-loading state, not changing config. Essentially 'display-reload' is about re-starting QEMU displays with the same config, while 'display-update' is about restarting with a new config. This shouldn't be too much work to achieve in your patch. Just clone everything related to the 'display-reload' QMP command boilerplate, replacing 'reload' with 'update' throughout and discarding the 'tls-certs' bits leaving only your 'addresses' bit. We can use this 'display-update' command for making pasword and auth config changes possible too later. > > ## > # @DisplayReloadOptions: > diff --git a/include/ui/console.h b/include/ui/console.h > index f590819880..b052027915 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -512,7 +512,7 @@ int vnc_display_password(const char *id, const char *password); > int vnc_display_pw_expire(const char *id, time_t expires); > void vnc_parse(const char *str); > int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp); > -bool vnc_display_reload_certs(const char *id, Error **errp); > +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp); > > /* input.c */ > int index_from_key(const char *key, size_t key_length); > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c > index 14e3beeaaf..ad45baa12b 100644 > --- a/monitor/qmp-cmds.c > +++ b/monitor/qmp-cmds.c > @@ -356,9 +356,7 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp) > switch (arg->type) { > case DISPLAY_RELOAD_TYPE_VNC: > #ifdef CONFIG_VNC > - if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) { > - vnc_display_reload_certs(NULL, errp); > - } > + vnc_display_reload(&arg->u.vnc, errp); > #else > error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'"); > #endif > diff --git a/ui/vnc.c b/ui/vnc.c > index fa0fb736d3..a86bd6335e 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -587,16 +587,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) > return prev; > } > > -bool vnc_display_reload_certs(const char *id, Error **errp) > +static bool vnc_display_reload_certs(VncDisplay *vd, Error **errp) > { > - VncDisplay *vd = vnc_display_find(id); > QCryptoTLSCredsClass *creds = NULL; > > - if (!vd) { > - error_setg(errp, "Can not find vnc display"); > - return false; > - } > - > if (!vd->tlscreds) { > error_setg(errp, "vnc tls is not enabled"); > return false; > @@ -3973,6 +3967,35 @@ static int vnc_display_listen(VncDisplay *vd, > return 0; > } > > +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp) > +{ > + VncDisplay *vd = vnc_display_find(NULL); > + > + if (!vd) { > + error_setg(errp, "Can not find vnc display"); > + return false; > + } > + > + if (arg->has_tls_certs && arg->tls_certs) { > + if (!vnc_display_reload_certs(vd, errp)) { > + return false; > + } > + } > + > + if (arg->has_addresses) { > + if (vd->listener) { > + qio_net_listener_disconnect(vd->listener); > + object_unref(OBJECT(vd->listener)); > + vd->listener = NULL; > + } > + > + if (vnc_display_listen(vd, arg->addresses, NULL, errp) < 0) { > + return false; > + } > + } > + > + return true; > +} > > void vnc_display_open(const char *id, Error **errp) > { > -- > 2.31.1 > Regards, Daniel
09.02.2022 21:33, Daniel P. Berrangé wrote: > On Tue, Jan 18, 2022 at 05:09:08PM +0100, Vladimir Sementsov-Ogievskiy wrote: >> Add possibility to 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 | 6 +++++- >> include/ui/console.h | 2 +- >> monitor/qmp-cmds.c | 4 +--- >> ui/vnc.c | 37 ++++++++++++++++++++++++++------- >> 5 files changed, 39 insertions(+), 13 deletions(-) >> >> diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst >> index 4c4da20d0f..b92626a74e 100644 >> --- a/docs/about/removed-features.rst >> +++ b/docs/about/removed-features.rst >> @@ -355,7 +355,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 >> +``display-reload`` instead. >> >> ``query-events`` (removed in 6.0) >> ''''''''''''''''''''''''''''''''' >> diff --git a/qapi/ui.json b/qapi/ui.json >> index 9354f4c467..4c4448f378 100644 >> --- a/qapi/ui.json >> +++ b/qapi/ui.json >> @@ -1293,12 +1293,16 @@ >> # Specify the VNC reload options. >> # >> # @tls-certs: reload tls certs or not. >> +# @addresses: If specified, change set of addresses >> +# to listen for connections. Addresses configured >> +# for websockets are not touched. (since 7.0) >> # >> # Since: 6.0 >> # >> ## >> { 'struct': 'DisplayReloadOptionsVNC', >> - 'data': { '*tls-certs': 'bool' } } >> + 'data': { '*tls-certs': 'bool', >> + '*addresses': ['SocketAddress'] } } > > So I'm having second thoughts on this, because I think we in fact need to > distinguish between reloads of state related to existing configuration > vs applying changes to the actual configuration. > > For example, when I think about the 'tls-certs' option here, it > lets us reload the existing TLS credentials from disk. ie it lets > the user re-write the TLS file content on disk and then tell QEMU > to reload the files. > > An equally valuable option would be to tell QEMU to simply use a > completely different set of TLS files on disk, rather than re-writing > in place. We don't have this feature now, but I think we should > anticipate it in the design. > > So I'm going to suggest that instead of 'display-reload', we should > have a general purpose 'display-update' command for modifying existing > config and , leave 'display-reload' purely for re-loading state, not > changing config. > > Essentially 'display-reload' is about re-starting QEMU displays > with the same config, while 'display-update' is about restarting > with a new config. > > This shouldn't be too much work to achieve in your patch. Just > clone everything related to the 'display-reload' QMP command > boilerplate, replacing 'reload' with 'update' throughout and > discarding the 'tls-certs' bits leaving only your 'addresses' > bit. Yes, that's simple to do, I'll resend soon. > > We can use this 'display-update' command for making pasword > and auth config changes possible too later. > >> >> ## >> # @DisplayReloadOptions: >> diff --git a/include/ui/console.h b/include/ui/console.h >> index f590819880..b052027915 100644 >> --- a/include/ui/console.h >> +++ b/include/ui/console.h >> @@ -512,7 +512,7 @@ int vnc_display_password(const char *id, const char *password); >> int vnc_display_pw_expire(const char *id, time_t expires); >> void vnc_parse(const char *str); >> int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp); >> -bool vnc_display_reload_certs(const char *id, Error **errp); >> +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp); >> >> /* input.c */ >> int index_from_key(const char *key, size_t key_length); >> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c >> index 14e3beeaaf..ad45baa12b 100644 >> --- a/monitor/qmp-cmds.c >> +++ b/monitor/qmp-cmds.c >> @@ -356,9 +356,7 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp) >> switch (arg->type) { >> case DISPLAY_RELOAD_TYPE_VNC: >> #ifdef CONFIG_VNC >> - if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) { >> - vnc_display_reload_certs(NULL, errp); >> - } >> + vnc_display_reload(&arg->u.vnc, errp); >> #else >> error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'"); >> #endif >> diff --git a/ui/vnc.c b/ui/vnc.c >> index fa0fb736d3..a86bd6335e 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -587,16 +587,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) >> return prev; >> } >> >> -bool vnc_display_reload_certs(const char *id, Error **errp) >> +static bool vnc_display_reload_certs(VncDisplay *vd, Error **errp) >> { >> - VncDisplay *vd = vnc_display_find(id); >> QCryptoTLSCredsClass *creds = NULL; >> >> - if (!vd) { >> - error_setg(errp, "Can not find vnc display"); >> - return false; >> - } >> - >> if (!vd->tlscreds) { >> error_setg(errp, "vnc tls is not enabled"); >> return false; >> @@ -3973,6 +3967,35 @@ static int vnc_display_listen(VncDisplay *vd, >> return 0; >> } >> >> +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp) >> +{ >> + VncDisplay *vd = vnc_display_find(NULL); >> + >> + if (!vd) { >> + error_setg(errp, "Can not find vnc display"); >> + return false; >> + } >> + >> + if (arg->has_tls_certs && arg->tls_certs) { >> + if (!vnc_display_reload_certs(vd, errp)) { >> + return false; >> + } >> + } >> + >> + if (arg->has_addresses) { >> + if (vd->listener) { >> + qio_net_listener_disconnect(vd->listener); >> + object_unref(OBJECT(vd->listener)); >> + vd->listener = NULL; >> + } >> + >> + if (vnc_display_listen(vd, arg->addresses, NULL, errp) < 0) { >> + return false; >> + } >> + } >> + >> + return true; >> +} >> >> void vnc_display_open(const char *id, Error **errp) >> { >> -- >> 2.31.1 >> > > Regards, > Daniel >
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index 4c4da20d0f..b92626a74e 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -355,7 +355,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 +``display-reload`` instead. ``query-events`` (removed in 6.0) ''''''''''''''''''''''''''''''''' diff --git a/qapi/ui.json b/qapi/ui.json index 9354f4c467..4c4448f378 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -1293,12 +1293,16 @@ # Specify the VNC reload options. # # @tls-certs: reload tls certs or not. +# @addresses: If specified, change set of addresses +# to listen for connections. Addresses configured +# for websockets are not touched. (since 7.0) # # Since: 6.0 # ## { 'struct': 'DisplayReloadOptionsVNC', - 'data': { '*tls-certs': 'bool' } } + 'data': { '*tls-certs': 'bool', + '*addresses': ['SocketAddress'] } } ## # @DisplayReloadOptions: diff --git a/include/ui/console.h b/include/ui/console.h index f590819880..b052027915 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -512,7 +512,7 @@ int vnc_display_password(const char *id, const char *password); int vnc_display_pw_expire(const char *id, time_t expires); void vnc_parse(const char *str); int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp); -bool vnc_display_reload_certs(const char *id, Error **errp); +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp); /* input.c */ int index_from_key(const char *key, size_t key_length); diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index 14e3beeaaf..ad45baa12b 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -356,9 +356,7 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp) switch (arg->type) { case DISPLAY_RELOAD_TYPE_VNC: #ifdef CONFIG_VNC - if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) { - vnc_display_reload_certs(NULL, errp); - } + vnc_display_reload(&arg->u.vnc, errp); #else error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'"); #endif diff --git a/ui/vnc.c b/ui/vnc.c index fa0fb736d3..a86bd6335e 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -587,16 +587,10 @@ VncInfo2List *qmp_query_vnc_servers(Error **errp) return prev; } -bool vnc_display_reload_certs(const char *id, Error **errp) +static bool vnc_display_reload_certs(VncDisplay *vd, Error **errp) { - VncDisplay *vd = vnc_display_find(id); QCryptoTLSCredsClass *creds = NULL; - if (!vd) { - error_setg(errp, "Can not find vnc display"); - return false; - } - if (!vd->tlscreds) { error_setg(errp, "vnc tls is not enabled"); return false; @@ -3973,6 +3967,35 @@ static int vnc_display_listen(VncDisplay *vd, return 0; } +bool vnc_display_reload(DisplayReloadOptionsVNC *arg, Error **errp) +{ + VncDisplay *vd = vnc_display_find(NULL); + + if (!vd) { + error_setg(errp, "Can not find vnc display"); + return false; + } + + if (arg->has_tls_certs && arg->tls_certs) { + if (!vnc_display_reload_certs(vd, errp)) { + return false; + } + } + + if (arg->has_addresses) { + if (vd->listener) { + qio_net_listener_disconnect(vd->listener); + object_unref(OBJECT(vd->listener)); + vd->listener = NULL; + } + + if (vnc_display_listen(vd, arg->addresses, NULL, errp) < 0) { + return false; + } + } + + return true; +} void vnc_display_open(const char *id, Error **errp) {
Add possibility to 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 | 6 +++++- include/ui/console.h | 2 +- monitor/qmp-cmds.c | 4 +--- ui/vnc.c | 37 ++++++++++++++++++++++++++------- 5 files changed, 39 insertions(+), 13 deletions(-)