diff mbox

[5/8] ui: extract code to connect/listen from vnc_display_open

Message ID 20170105160701.22118-6-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Jan. 5, 2017, 4:06 p.m. UTC 
The code which takes a SocketAddress and connects/listens on the
network is going to get more complicated to deal with multiple
listeners. Pull it out into a separate method to avoid making the
vnc_display_open method even more complex.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc.c | 122 +++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 80 insertions(+), 42 deletions(-)

Comments

Eric Blake Jan. 6, 2017, 4 p.m. UTC | #1 
On 01/05/2017 10:06 AM, Daniel P. Berrange wrote:
> The code which takes a SocketAddress and connects/listens on the
> network is going to get more complicated to deal with multiple
> listeners. Pull it out into a separate method to avoid making the
> vnc_display_open method even more complex.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  ui/vnc.c | 122 +++++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 80 insertions(+), 42 deletions(-)
> 

> +static int vnc_display_listen(VncDisplay *vd,
> +                              SocketAddress *saddr,
> +                              SocketAddress *wsaddr,
> +                              Error **errp)
> +{
> +    vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
> +
> +    if (vnc_display_listen_addr(vd, saddr,
> +                                "vnc-listen",
> +                                &vd->lsock,
> +                                &vd->lsock_tag,
> +                                &vd->nlsock,
> +                                errp) < 0) {
> +        return -1;

If this succeeds,

> +    }
> +    if (wsaddr &&
> +        vnc_display_listen_addr(vd, wsaddr,
> +                                "vnc-ws-listen",
> +                                &vd->lwebsock,
> +                                &vd->lwebsock_tag,
> +                                &vd->nlwebsock,
> +                                errp) < 0) {
> +        return -1;

but this fails, you are relying on the caller to clean up the successful
port allocation.  Then again, in the old code:

> @@ -3909,53 +3987,13 @@ void vnc_display_open(const char *id, Error **errp)

>      } else {
> -        vd->nlsock = 1;
> -        vd->lsock = g_new0(QIOChannelSocket *, 1);
> -        vd->lsock_tag = g_new0(guint, 1);
> -
> -        vd->lsock[0] = qio_channel_socket_new();
> -        qio_channel_set_name(QIO_CHANNEL(vd->lsock[0]), "vnc-listen");
> -        if (qio_channel_socket_listen_sync(vd->lsock[0], saddr, errp) < 0) {
> +        if (vnc_display_listen(vd, saddr, wsaddr, errp) < 0) {
>              goto fail;
>          }
> -        vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
> -
> -        if (wsaddr) {
> -            vd->nlwebsock = 1;
> -            vd->lwebsock = g_new0(QIOChannelSocket *, 1);
> -            vd->lwebsock_tag = g_new0(guint, 1);
> -
> -            vd->lwebsock[0] = qio_channel_socket_new();
> -            qio_channel_set_name(QIO_CHANNEL(vd->lwebsock[0]), "vnc-ws-listen");
> -            if (qio_channel_socket_listen_sync(vd->lwebsock[0],
> -                                               wsaddr, errp) < 0) {
> -                goto fail;
> -            }

this is all the more cleanup you do locally:

fail:
    qapi_free_SocketAddress(saddr);
    qapi_free_SocketAddress(wsaddr);
    ws_enabled = false;

so you already have cleanup at a distance when you have partial
allocation before failure (that is, the caller cleans up the
partially-constructed 'vd' object when errp is set).  So you aren't
making it worse.  Thus

Reviewed-by: Eric Blake <eblake@redhat.com>
diff mbox

Patch

diff --git a/ui/vnc.c b/ui/vnc.c
index 8d3f9c1..94135c8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3691,6 +3691,84 @@  static int vnc_display_get_addresses(QemuOpts *opts,
     return -1;
 }
 
+static int vnc_display_connect(VncDisplay *vd,
+                               SocketAddress *saddr,
+                               SocketAddress *wsaddr,
+                               Error **errp)
+{
+    /* connect to viewer */
+    QIOChannelSocket *sioc = NULL;
+    if (wsaddr) {
+        error_setg(errp, "Cannot use websockets in reverse mode");
+        return -1;
+    }
+    vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
+    sioc = qio_channel_socket_new();
+    qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse");
+    if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
+        return -1;
+    }
+    vnc_connect(vd, sioc, false, false);
+    object_unref(OBJECT(sioc));
+    return 0;
+}
+
+
+static int vnc_display_listen_addr(VncDisplay *vd,
+                                   SocketAddress *addr,
+                                   const char *name,
+                                   QIOChannelSocket ***lsock,
+                                   guint **lsock_tag,
+                                   size_t *nlsock,
+                                   Error **errp)
+{
+    *nlsock = 1;
+    *lsock = g_new0(QIOChannelSocket *, 1);
+    *lsock_tag = g_new0(guint, 1);
+
+    (*lsock)[0] = qio_channel_socket_new();
+    qio_channel_set_name(QIO_CHANNEL((*lsock)[0]), name);
+    if (qio_channel_socket_listen_sync((*lsock)[0], addr, errp) < 0) {
+        return -1;
+    }
+
+    (*lsock_tag)[0] = qio_channel_add_watch(
+        QIO_CHANNEL((*lsock)[0]),
+        G_IO_IN, vnc_listen_io, vd, NULL);
+
+    return 0;
+}
+
+
+static int vnc_display_listen(VncDisplay *vd,
+                              SocketAddress *saddr,
+                              SocketAddress *wsaddr,
+                              Error **errp)
+{
+    vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
+
+    if (vnc_display_listen_addr(vd, saddr,
+                                "vnc-listen",
+                                &vd->lsock,
+                                &vd->lsock_tag,
+                                &vd->nlsock,
+                                errp) < 0) {
+        return -1;
+    }
+    if (wsaddr &&
+        vnc_display_listen_addr(vd, wsaddr,
+                                "vnc-ws-listen",
+                                &vd->lwebsock,
+                                &vd->lwebsock_tag,
+                                &vd->nlwebsock,
+                                errp) < 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
+
 void vnc_display_open(const char *id, Error **errp)
 {
     VncDisplay *vd = vnc_display_find(id);
@@ -3909,53 +3987,13 @@  void vnc_display_open(const char *id, Error **errp)
     }
 
     if (reverse) {
-        /* connect to viewer */
-        QIOChannelSocket *sioc = NULL;
-        if (wsaddr) {
-            error_setg(errp, "Cannot use websockets in reverse mode");
-            goto fail;
-        }
-        vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
-        sioc = qio_channel_socket_new();
-        qio_channel_set_name(QIO_CHANNEL(sioc), "vnc-reverse");
-        if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
+        if (vnc_display_connect(vd, saddr, wsaddr, errp) < 0) {
             goto fail;
         }
-        vnc_connect(vd, sioc, false, false);
-        object_unref(OBJECT(sioc));
     } else {
-        vd->nlsock = 1;
-        vd->lsock = g_new0(QIOChannelSocket *, 1);
-        vd->lsock_tag = g_new0(guint, 1);
-
-        vd->lsock[0] = qio_channel_socket_new();
-        qio_channel_set_name(QIO_CHANNEL(vd->lsock[0]), "vnc-listen");
-        if (qio_channel_socket_listen_sync(vd->lsock[0], saddr, errp) < 0) {
+        if (vnc_display_listen(vd, saddr, wsaddr, errp) < 0) {
             goto fail;
         }
-        vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
-
-        if (wsaddr) {
-            vd->nlwebsock = 1;
-            vd->lwebsock = g_new0(QIOChannelSocket *, 1);
-            vd->lwebsock_tag = g_new0(guint, 1);
-
-            vd->lwebsock[0] = qio_channel_socket_new();
-            qio_channel_set_name(QIO_CHANNEL(vd->lwebsock[0]), "vnc-ws-listen");
-            if (qio_channel_socket_listen_sync(vd->lwebsock[0],
-                                               wsaddr, errp) < 0) {
-                goto fail;
-            }
-        }
-
-        vd->lsock_tag[0] = qio_channel_add_watch(
-            QIO_CHANNEL(vd->lsock[0]),
-            G_IO_IN, vnc_listen_io, vd, NULL);
-        if (wsaddr) {
-            vd->lwebsock_tag[0] = qio_channel_add_watch(
-                QIO_CHANNEL(vd->lwebsock[0]),
-                G_IO_IN, vnc_listen_io, vd, NULL);
-        }
     }
 
     if (qemu_opt_get(opts, "to")) {