Hi
On Mon, Dec 20, 2021 at 10:21 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> 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 <vsement...@virtuozzo.com>
>
Nice
Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
---
> ui/vnc.c | 129 ++++++++++++++++++++++---------------------------------
> 1 file changed, 51 insertions(+), 78 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index af02522e84..c9e26c70df 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
>
>
>
--
Marc-André Lureau
