diff mbox series

[1/2] ui/vnc: refactor arrays of addresses to SocketAddressList

Message ID 20211220154418.1554279-2-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series qapi/ui: add change-vnc-listen | expand

Commit Message

Vladimir Sementsov-Ogievskiy Dec. 20, 2021, 3:44 p.m. UTC 
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>
---
 ui/vnc.c | 129 ++++++++++++++++++++++---------------------------------
 1 file changed, 51 insertions(+), 78 deletions(-)

Comments

Marc-André Lureau Dec. 21, 2021, 7:56 a.m. UTC | #1 
Hi

On Mon, Dec 20, 2021 at 10:21 PM Vladimir Sementsov-Ogievskiy <
vsementsov@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 <vsementsov@virtuozzo.com>
>

Nice
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 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
>
>
>
diff mbox series

Patch

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)