* [PATCH v3 0/3] qapi/ui: change vnc addresses
@ 2022-01-18 16:09 Vladimir Sementsov-Ogievskiy
2022-01-18 16:09 ` [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList Vladimir Sementsov-Ogievskiy
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-18 16:09 UTC (permalink / raw)
To: qemu-devel
Cc: bleal, wainersm, f4bug, crosa, eblake, armbru, kraxel, berrange,
vsementsov, marcandre.lureau
Hi all!
v3: - instead of creating new qmp command add an argument to existing
display-reload
- also don't touch websockets for now. I'm not sure we need it.
Additional argument for changing websockets may be added later on
demand
Recently our customer requested a possibility to change VNC listen port
dynamically.
Happily in Rhel7-based Qemu we already have this possibility: through
deprecated "change" qmp command.
But since 6.0 "change" qmp command was removed, with recommendation to
use change-vnc-password or blockdev-change-medium instead. Of course,
neither of them allow change VNC listen port.
So, let's reimplement the possibility.
Note: now, reconnecting may trigger existing deadlock, as I described
in my message "Re: [PULL 09/11] ui/vnc: clipboard support":
<973ddebe-14a9-4ba7-c389-7a97d6017237@virtuozzo.com>
Simple hack helps, but I'm not sure it's safe itself:
diff --git a/ui/vnc.c b/ui/vnc.c
index 69bbf3b6f6..8c6b378e2e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1354,12 +1354,12 @@ void vnc_disconnect_finish(VncState *vs)
/* last client gone */
vnc_update_server_surface(vs->vd);
}
+ vnc_unlock_output(vs);
+
if (vs->cbpeer.update.notify) {
qemu_clipboard_peer_unregister(&vs->cbpeer);
}
- vnc_unlock_output(vs);
-
qemu_mutex_destroy(&vs->output_mutex);
if (vs->bh != NULL) {
qemu_bh_delete(vs->bh);
Vladimir Sementsov-Ogievskiy (3):
ui/vnc: refactor arrays of addresses to SocketAddressList
qapi/ui: display-reload: add possibility to change listen address
avocado/vnc: add test_change_listen
docs/about/removed-features.rst | 3 +-
qapi/ui.json | 6 +-
include/ui/console.h | 2 +-
monitor/qmp-cmds.c | 4 +-
ui/vnc.c | 166 ++++++++++++++++----------------
tests/avocado/vnc.py | 10 ++
6 files changed, 100 insertions(+), 91 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList
2022-01-18 16:09 [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
@ 2022-01-18 16:09 ` Vladimir Sementsov-Ogievskiy
2022-02-09 18:22 ` Daniel P. Berrangé
2022-01-18 16:09 ` [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-18 16:09 UTC (permalink / raw)
To: qemu-devel
Cc: bleal, wainersm, f4bug, crosa, eblake, armbru, kraxel, berrange,
vsementsov, marcandre.lureau
Let's use SocketAddressList instead of dynamic arrays.
Benefits:
- Automatic cleanup: don't need specific freeing function and drop
some gotos.
- Less indirection: no triple asterix anymore!
- Prepare for the following commit, which will reuse new interface of
vnc_display_listen().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
ui/vnc.c | 129 ++++++++++++++++++++++---------------------------------
1 file changed, 51 insertions(+), 78 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 3ccd33dedc..fa0fb736d3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3812,30 +3812,19 @@ static int vnc_display_get_address(const char *addrstr,
return ret;
}
-static void vnc_free_addresses(SocketAddress ***retsaddr,
- size_t *retnsaddr)
-{
- size_t i;
-
- for (i = 0; i < *retnsaddr; i++) {
- qapi_free_SocketAddress((*retsaddr)[i]);
- }
- g_free(*retsaddr);
-
- *retsaddr = NULL;
- *retnsaddr = 0;
-}
-
static int vnc_display_get_addresses(QemuOpts *opts,
bool reverse,
- SocketAddress ***retsaddr,
- size_t *retnsaddr,
- SocketAddress ***retwsaddr,
- size_t *retnwsaddr,
+ SocketAddressList **saddr_list_ret,
+ SocketAddressList **wsaddr_list_ret,
Error **errp)
{
SocketAddress *saddr = NULL;
SocketAddress *wsaddr = NULL;
+ g_autoptr(SocketAddressList) saddr_list = NULL;
+ SocketAddressList **saddr_tail = &saddr_list;
+ SocketAddress *single_saddr = NULL;
+ g_autoptr(SocketAddressList) wsaddr_list = NULL;
+ SocketAddressList **wsaddr_tail = &wsaddr_list;
QemuOptsIter addriter;
const char *addr;
int to = qemu_opt_get_number(opts, "to", 0);
@@ -3844,23 +3833,16 @@ static int vnc_display_get_addresses(QemuOpts *opts,
bool ipv4 = qemu_opt_get_bool(opts, "ipv4", false);
bool ipv6 = qemu_opt_get_bool(opts, "ipv6", false);
int displaynum = -1;
- int ret = -1;
-
- *retsaddr = NULL;
- *retnsaddr = 0;
- *retwsaddr = NULL;
- *retnwsaddr = 0;
addr = qemu_opt_get(opts, "vnc");
if (addr == NULL || g_str_equal(addr, "none")) {
- ret = 0;
- goto cleanup;
+ return 0;
}
if (qemu_opt_get(opts, "websocket") &&
!qcrypto_hash_supports(QCRYPTO_HASH_ALG_SHA1)) {
error_setg(errp,
"SHA1 hash support is required for websockets");
- goto cleanup;
+ return -1;
}
qemu_opt_iter_init(&addriter, opts, "vnc");
@@ -3871,7 +3853,7 @@ static int vnc_display_get_addresses(QemuOpts *opts,
ipv4, ipv6,
&saddr, errp);
if (rv < 0) {
- goto cleanup;
+ return -1;
}
/* Historical compat - first listen address can be used
* to set the default websocket port
@@ -3879,13 +3861,16 @@ static int vnc_display_get_addresses(QemuOpts *opts,
if (displaynum == -1) {
displaynum = rv;
}
- *retsaddr = g_renew(SocketAddress *, *retsaddr, *retnsaddr + 1);
- (*retsaddr)[(*retnsaddr)++] = saddr;
+ QAPI_LIST_APPEND(saddr_tail, saddr);
}
- /* If we had multiple primary displays, we don't do defaults
- * for websocket, and require explicit config instead. */
- if (*retnsaddr > 1) {
+ if (saddr_list && !saddr_list->next) {
+ single_saddr = saddr_list->value;
+ } else {
+ /*
+ * If we had multiple primary displays, we don't do defaults
+ * for websocket, and require explicit config instead.
+ */
displaynum = -1;
}
@@ -3895,57 +3880,50 @@ static int vnc_display_get_addresses(QemuOpts *opts,
has_ipv4, has_ipv6,
ipv4, ipv6,
&wsaddr, errp) < 0) {
- goto cleanup;
+ return -1;
}
/* Historical compat - if only a single listen address was
* provided, then this is used to set the default listen
* address for websocket too
*/
- if (*retnsaddr == 1 &&
- (*retsaddr)[0]->type == SOCKET_ADDRESS_TYPE_INET &&
+ if (single_saddr &&
+ single_saddr->type == SOCKET_ADDRESS_TYPE_INET &&
wsaddr->type == SOCKET_ADDRESS_TYPE_INET &&
g_str_equal(wsaddr->u.inet.host, "") &&
- !g_str_equal((*retsaddr)[0]->u.inet.host, "")) {
+ !g_str_equal(single_saddr->u.inet.host, "")) {
g_free(wsaddr->u.inet.host);
- wsaddr->u.inet.host = g_strdup((*retsaddr)[0]->u.inet.host);
+ wsaddr->u.inet.host = g_strdup(single_saddr->u.inet.host);
}
- *retwsaddr = g_renew(SocketAddress *, *retwsaddr, *retnwsaddr + 1);
- (*retwsaddr)[(*retnwsaddr)++] = wsaddr;
+ QAPI_LIST_APPEND(wsaddr_tail, wsaddr);
}
- ret = 0;
- cleanup:
- if (ret < 0) {
- vnc_free_addresses(retsaddr, retnsaddr);
- vnc_free_addresses(retwsaddr, retnwsaddr);
- }
- return ret;
+ *saddr_list_ret = g_steal_pointer(&saddr_list);
+ *wsaddr_list_ret = g_steal_pointer(&wsaddr_list);
+ return 0;
}
static int vnc_display_connect(VncDisplay *vd,
- SocketAddress **saddr,
- size_t nsaddr,
- SocketAddress **wsaddr,
- size_t nwsaddr,
+ SocketAddressList *saddr_list,
+ SocketAddressList *wsaddr_list,
Error **errp)
{
/* connect to viewer */
QIOChannelSocket *sioc = NULL;
- if (nwsaddr != 0) {
+ if (wsaddr_list) {
error_setg(errp, "Cannot use websockets in reverse mode");
return -1;
}
- if (nsaddr != 1) {
+ if (!saddr_list || saddr_list->next) {
error_setg(errp, "Expected a single address in reverse mode");
return -1;
}
/* TODO SOCKET_ADDRESS_TYPE_FD when fd has AF_UNIX */
- vd->is_unix = saddr[0]->type == SOCKET_ADDRESS_TYPE_UNIX;
+ vd->is_unix = saddr_list->value->type == SOCKET_ADDRESS_TYPE_UNIX;
sioc = qio_channel_socket_new();
qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse");
- if (qio_channel_socket_connect_sync(sioc, saddr[0], errp) < 0) {
+ if (qio_channel_socket_connect_sync(sioc, saddr_list->value, errp) < 0) {
object_unref(OBJECT(sioc));
return -1;
}
@@ -3956,20 +3934,18 @@ static int vnc_display_connect(VncDisplay *vd,
static int vnc_display_listen(VncDisplay *vd,
- SocketAddress **saddr,
- size_t nsaddr,
- SocketAddress **wsaddr,
- size_t nwsaddr,
+ SocketAddressList *saddr_list,
+ SocketAddressList *wsaddr_list,
Error **errp)
{
- size_t i;
+ SocketAddressList *el;
- if (nsaddr) {
+ if (saddr_list) {
vd->listener = qio_net_listener_new();
qio_net_listener_set_name(vd->listener, "vnc-listen");
- for (i = 0; i < nsaddr; i++) {
+ for (el = saddr_list; el; el = el->next) {
if (qio_net_listener_open_sync(vd->listener,
- saddr[i], 1,
+ el->value, 1,
errp) < 0) {
return -1;
}
@@ -3979,12 +3955,12 @@ static int vnc_display_listen(VncDisplay *vd,
vnc_listen_io, vd, NULL);
}
- if (nwsaddr) {
+ if (wsaddr_list) {
vd->wslistener = qio_net_listener_new();
qio_net_listener_set_name(vd->wslistener, "vnc-ws-listen");
- for (i = 0; i < nwsaddr; i++) {
+ for (el = wsaddr_list; el; el = el->next) {
if (qio_net_listener_open_sync(vd->wslistener,
- wsaddr[i], 1,
+ el->value, 1,
errp) < 0) {
return -1;
}
@@ -4002,8 +3978,8 @@ void vnc_display_open(const char *id, Error **errp)
{
VncDisplay *vd = vnc_display_find(id);
QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
- SocketAddress **saddr = NULL, **wsaddr = NULL;
- size_t nsaddr, nwsaddr;
+ g_autoptr(SocketAddressList) saddr_list = NULL;
+ g_autoptr(SocketAddressList) wsaddr_list = NULL;
const char *share, *device_id;
QemuConsole *con;
bool password = false;
@@ -4028,8 +4004,8 @@ void vnc_display_open(const char *id, Error **errp)
}
reverse = qemu_opt_get_bool(opts, "reverse", false);
- if (vnc_display_get_addresses(opts, reverse, &saddr, &nsaddr,
- &wsaddr, &nwsaddr, errp) < 0) {
+ if (vnc_display_get_addresses(opts, reverse, &saddr_list, &wsaddr_list,
+ errp) < 0) {
goto fail;
}
@@ -4211,16 +4187,16 @@ void vnc_display_open(const char *id, Error **errp)
}
qkbd_state_set_delay(vd->kbd, key_delay_ms);
- if (saddr == NULL) {
- goto cleanup;
+ if (saddr_list == NULL) {
+ return;
}
if (reverse) {
- if (vnc_display_connect(vd, saddr, nsaddr, wsaddr, nwsaddr, errp) < 0) {
+ if (vnc_display_connect(vd, saddr_list, wsaddr_list, errp) < 0) {
goto fail;
}
} else {
- if (vnc_display_listen(vd, saddr, nsaddr, wsaddr, nwsaddr, errp) < 0) {
+ if (vnc_display_listen(vd, saddr_list, wsaddr_list, errp) < 0) {
goto fail;
}
}
@@ -4229,14 +4205,11 @@ void vnc_display_open(const char *id, Error **errp)
vnc_display_print_local_addr(vd);
}
- cleanup:
- vnc_free_addresses(&saddr, &nsaddr);
- vnc_free_addresses(&wsaddr, &nwsaddr);
+ /* Success */
return;
fail:
vnc_display_close(vd);
- goto cleanup;
}
void vnc_display_add_client(const char *id, int csock, bool skipauth)
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address
2022-01-18 16:09 [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
2022-01-18 16:09 ` [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList Vladimir Sementsov-Ogievskiy
@ 2022-01-18 16:09 ` Vladimir Sementsov-Ogievskiy
2022-02-09 18:33 ` Daniel P. Berrangé
2022-01-18 16:09 ` [PATCH v3 3/3] avocado/vnc: add test_change_listen Vladimir Sementsov-Ogievskiy
2022-02-09 16:55 ` [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
3 siblings, 1 reply; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-18 16:09 UTC (permalink / raw)
To: qemu-devel
Cc: bleal, wainersm, f4bug, crosa, eblake, armbru, kraxel, berrange,
vsementsov, marcandre.lureau
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'] } }
##
# @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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] avocado/vnc: add test_change_listen
2022-01-18 16:09 [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
2022-01-18 16:09 ` [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList Vladimir Sementsov-Ogievskiy
2022-01-18 16:09 ` [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address Vladimir Sementsov-Ogievskiy
@ 2022-01-18 16:09 ` Vladimir Sementsov-Ogievskiy
2022-02-09 18:38 ` Daniel P. Berrangé
2022-02-09 16:55 ` [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
3 siblings, 1 reply; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-18 16:09 UTC (permalink / raw)
To: qemu-devel
Cc: bleal, wainersm, f4bug, crosa, eblake, armbru, kraxel, berrange,
vsementsov, marcandre.lureau
Add simple test-case for new addresses argument of display-reload qmp
command.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/avocado/vnc.py | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tests/avocado/vnc.py b/tests/avocado/vnc.py
index 096432988f..936285a50b 100644
--- a/tests/avocado/vnc.py
+++ b/tests/avocado/vnc.py
@@ -51,3 +51,13 @@ def test_change_password(self):
set_password_response = self.vm.qmp('change-vnc-password',
password='new_password')
self.assertEqual(set_password_response['return'], {})
+
+ def test_change_listen(self):
+ self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
+ self.vm.launch()
+ self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5900')
+ res = self.vm.qmp('display-reload', type='vnc',
+ addresses=[{'type': 'inet', 'host': '0.0.0.0',
+ 'port': '5901'}])
+ self.assertEqual(res['return'], {})
+ self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5901')
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/3] qapi/ui: change vnc addresses
2022-01-18 16:09 [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2022-01-18 16:09 ` [PATCH v3 3/3] avocado/vnc: add test_change_listen Vladimir Sementsov-Ogievskiy
@ 2022-02-09 16:55 ` Vladimir Sementsov-Ogievskiy
3 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-02-09 16:55 UTC (permalink / raw)
To: qemu-devel
Cc: bleal, wainersm, f4bug, crosa, eblake, armbru, kraxel, berrange,
marcandre.lureau
Ping)
18.01.2022 19:09, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> v3: - instead of creating new qmp command add an argument to existing
> display-reload
> - also don't touch websockets for now. I'm not sure we need it.
> Additional argument for changing websockets may be added later on
> demand
>
> Recently our customer requested a possibility to change VNC listen port
> dynamically.
>
> Happily in Rhel7-based Qemu we already have this possibility: through
> deprecated "change" qmp command.
>
> But since 6.0 "change" qmp command was removed, with recommendation to
> use change-vnc-password or blockdev-change-medium instead. Of course,
> neither of them allow change VNC listen port.
>
> So, let's reimplement the possibility.
>
> Note: now, reconnecting may trigger existing deadlock, as I described
> in my message "Re: [PULL 09/11] ui/vnc: clipboard support":
> <973ddebe-14a9-4ba7-c389-7a97d6017237@virtuozzo.com>
>
> Simple hack helps, but I'm not sure it's safe itself:
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 69bbf3b6f6..8c6b378e2e 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1354,12 +1354,12 @@ void vnc_disconnect_finish(VncState *vs)
> /* last client gone */
> vnc_update_server_surface(vs->vd);
> }
> + vnc_unlock_output(vs);
> +
> if (vs->cbpeer.update.notify) {
> qemu_clipboard_peer_unregister(&vs->cbpeer);
> }
>
> - vnc_unlock_output(vs);
> -
> qemu_mutex_destroy(&vs->output_mutex);
> if (vs->bh != NULL) {
> qemu_bh_delete(vs->bh);
>
> Vladimir Sementsov-Ogievskiy (3):
> ui/vnc: refactor arrays of addresses to SocketAddressList
> qapi/ui: display-reload: add possibility to change listen address
> avocado/vnc: add test_change_listen
>
> docs/about/removed-features.rst | 3 +-
> qapi/ui.json | 6 +-
> include/ui/console.h | 2 +-
> monitor/qmp-cmds.c | 4 +-
> ui/vnc.c | 166 ++++++++++++++++----------------
> tests/avocado/vnc.py | 10 ++
> 6 files changed, 100 insertions(+), 91 deletions(-)
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList
2022-01-18 16:09 ` [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList Vladimir Sementsov-Ogievskiy
@ 2022-02-09 18:22 ` Daniel P. Berrangé
0 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-02-09 18:22 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: bleal, armbru, f4bug, wainersm, qemu-devel, kraxel, crosa,
marcandre.lureau, eblake
On Tue, Jan 18, 2022 at 05:09:07PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Let's use SocketAddressList instead of dynamic arrays.
> Benefits:
> - Automatic cleanup: don't need specific freeing function and drop
> some gotos.
> - Less indirection: no triple asterix anymore!
> - Prepare for the following commit, which will reuse new interface of
> vnc_display_listen().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> ui/vnc.c | 129 ++++++++++++++++++++++---------------------------------
> 1 file changed, 51 insertions(+), 78 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address
2022-01-18 16:09 ` [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address Vladimir Sementsov-Ogievskiy
@ 2022-02-09 18:33 ` Daniel P. Berrangé
2022-02-10 9:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-02-09 18:33 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: bleal, armbru, f4bug, wainersm, qemu-devel, kraxel, crosa,
marcandre.lureau, eblake
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] avocado/vnc: add test_change_listen
2022-01-18 16:09 ` [PATCH v3 3/3] avocado/vnc: add test_change_listen Vladimir Sementsov-Ogievskiy
@ 2022-02-09 18:38 ` Daniel P. Berrangé
0 siblings, 0 replies; 9+ messages in thread
From: Daniel P. Berrangé @ 2022-02-09 18:38 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: bleal, armbru, f4bug, wainersm, qemu-devel, kraxel, crosa,
marcandre.lureau, eblake
On Tue, Jan 18, 2022 at 05:09:09PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Add simple test-case for new addresses argument of display-reload qmp
> command.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> tests/avocado/vnc.py | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/tests/avocado/vnc.py b/tests/avocado/vnc.py
> index 096432988f..936285a50b 100644
> --- a/tests/avocado/vnc.py
> +++ b/tests/avocado/vnc.py
> @@ -51,3 +51,13 @@ def test_change_password(self):
> set_password_response = self.vm.qmp('change-vnc-password',
> password='new_password')
> self.assertEqual(set_password_response['return'], {})
> +
> + def test_change_listen(self):
> + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
> + self.vm.launch()
> + self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5900')
Add a check that socket connect(5900) works here and connect(5901)
and connect(5902) fails...
> + res = self.vm.qmp('display-reload', type='vnc',
> + addresses=[{'type': 'inet', 'host': '0.0.0.0',
> + 'port': '5901'}])
I'd suggest we provide multiple addresses to test multi address
changes on distinct IPs too
addresses=[{'type': 'inet', 'host': '0.0.0.0',
'port': '5901'},
{'type': 'inet', 'host': '127.0.0.1',
'port': '5902'}]
> + self.assertEqual(res['return'], {})
> + self.assertEqual(self.vm.qmp('query-vnc')['return']['service'], '5901')
and connect(5900) fails here, and connect(5901) + connet(5902) works .
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address
2022-02-09 18:33 ` Daniel P. Berrangé
@ 2022-02-10 9:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-02-10 9:39 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, bleal, wainersm, f4bug, crosa, eblake, armbru,
kraxel, marcandre.lureau
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
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-02-10 9:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-18 16:09 [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
2022-01-18 16:09 ` [PATCH v3 1/3] ui/vnc: refactor arrays of addresses to SocketAddressList Vladimir Sementsov-Ogievskiy
2022-02-09 18:22 ` Daniel P. Berrangé
2022-01-18 16:09 ` [PATCH v3 2/3] qapi/ui: display-reload: add possibility to change listen address Vladimir Sementsov-Ogievskiy
2022-02-09 18:33 ` Daniel P. Berrangé
2022-02-10 9:39 ` Vladimir Sementsov-Ogievskiy
2022-01-18 16:09 ` [PATCH v3 3/3] avocado/vnc: add test_change_listen Vladimir Sementsov-Ogievskiy
2022-02-09 18:38 ` Daniel P. Berrangé
2022-02-09 16:55 ` [PATCH v3 0/3] qapi/ui: change vnc addresses Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.