[Spice-devel,spice-gtk,6/6] spice-session: make spice_session_channel_open_host() async again

Submitted by Marc-André Lureau on Aug. 24, 2012, 1:58 p.m.

Details

Message ID 1345816699-24989-7-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marc-André Lureau Aug. 24, 2012, 1:58 p.m.
The previous patches (introducing GSocketClient & GProxy usage) turned
the code into only sync calls.

By using async() variants of resolve & connect functions, make the
open host connection asynchronous again.
---
 gtk/spice-session.c | 195 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 123 insertions(+), 72 deletions(-)

Patch hide | download patch | download mbox

diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index ae70b73..1688c58 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -28,6 +28,7 @@ 
 #include "gio-coroutine.h"
 #include "glib-compat.h"
 #include "wocky-http-proxy.h"
+#include "spice-proxy.h"
 
 struct channel {
     SpiceChannel      *channel;
@@ -1511,59 +1512,123 @@  gboolean spice_session_has_channel_type(SpiceSession *session, gint type)
 /* ------------------------------------------------------------------ */
 /* private functions                                                  */
 
-static GSocketAddress* get_proxy_address(SpiceSession *session, guint port, GError **error)
+static SpiceProxy* get_proxy(GError **error)
 {
-    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
-    GSocketAddress *address = NULL;
-    const gchar *proxy = g_getenv("SPICE_PROXY");
-    GList *addresses = NULL, *it;
+    SpiceProxy *proxy;
 
-    if (proxy == NULL)
+    const gchar *proxy_env = g_getenv("SPICE_PROXY");
+    if (proxy_env == NULL || strlen(proxy_env) == 0)
         return NULL;
 
-    /* FIXME: use GUri when it is ready... */
-    if (g_ascii_strncasecmp("http://", proxy, 7) == 0)
-        proxy += 7;
-
-    gchar **proxyv = g_strsplit(proxy, ":", 0);
-    guint16 pport = 3128;
-    const gchar *proxy_host = NULL, *proxy_port = NULL;
-
-    proxy_host = proxyv[0];
-    if (proxy_host != NULL)
-        proxy_port = proxyv[1];
-
-    if (proxy_port != NULL) {
-        char *endptr;
-        pport = strtoul(proxy_port, &endptr, 10);
-        if (*endptr != '\0') {
-            g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
-                        "Invalid proxy port: %s", proxy_port);
-            goto end;
-        }
-    }
+    proxy = spice_proxy_new();
+    if (!spice_proxy_parse(proxy, proxy_env, error))
+        g_clear_object(&proxy);
+
+    return proxy;
+}
+
+typedef struct spice_open_host spice_open_host;
 
-    addresses = g_resolver_lookup_by_name(g_resolver_get_default(),
-                                          proxy_host, NULL, error);
-    if (*error != NULL)
+struct spice_open_host {
+    struct coroutine *from;
+    SpiceSession *session;
+    SpiceChannel *channel;
+    SpiceProxy *proxy;
+    int port;
+    GCancellable *cancellable;
+    GError *error;
+    GSocket *socket;
+};
+
+static void socket_client_connect_ready(GObject *source_object, GAsyncResult *result,
+                                        gpointer data)
+{
+    GSocketClient *client = G_SOCKET_CLIENT(source_object);
+    spice_open_host *open_host = data;
+    GSocketConnection *connection = NULL;
+
+    SPICE_DEBUG("connect ready");
+    connection = g_socket_client_connect_finish(client, result, &open_host->error);
+    if (connection == NULL)
         goto end;
 
-    /* FIXME: iterate over all addresses
-     * gproxy makes it quite unconvenient to deal with hostname,
-     * it would make sense to do it once earlier?
-     */
+    open_host->socket = g_socket_connection_get_socket(connection);
+    g_object_ref(open_host->socket);
+
+end:
+    g_object_unref(connection);
+    g_object_unref(client);
+
+    coroutine_yieldto(open_host->from, NULL);
+}
+
+/* main context */
+static void open_host_connectable_connect(spice_open_host *open_host, GSocketConnectable *connectable)
+{
+    GSocketClient *client;
+
+    SPICE_DEBUG("connecting %p...", open_host);
+    client = g_socket_client_new();
+    g_socket_client_connect_async(client, connectable, open_host->cancellable,
+                                  socket_client_connect_ready, open_host);
+}
+
+/* main context */
+static void proxy_lookup_ready(GObject *source_object, GAsyncResult *result,
+                               gpointer data)
+{
+    spice_open_host *open_host = data;
+    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(open_host->session);
+    GList *addresses = NULL, *it;
+    GSocketAddress *address;
+
+    SPICE_DEBUG("proxy lookup ready");
+    addresses = g_resolver_lookup_by_name_finish(G_RESOLVER(source_object),
+                                                 result, &open_host->error);
+    if (addresses == NULL || open_host->error) {
+        coroutine_yieldto(open_host->from, NULL);
+        return;
+    }
+
     for (it = addresses; it != NULL; it = it->next) {
-        address = g_proxy_address_new(G_INET_ADDRESS(it->data), pport, "http",
-                                      s->host, port, NULL, NULL);
+        address = g_proxy_address_new(G_INET_ADDRESS(it->data),
+                                      spice_proxy_get_port(open_host->proxy), "http",
+                                      s->host, open_host->port, NULL, NULL);
         if (address != NULL)
             break;
     }
 
-end:
+    open_host_connectable_connect(open_host, G_SOCKET_CONNECTABLE(address));
     g_resolver_free_addresses(addresses);
-    g_strfreev(proxyv);
+}
+
+/* main context */
+static gboolean open_host_idle_cb(gpointer data)
+{
+    spice_open_host *open_host = data;
+    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(open_host->session);
+
+    g_return_val_if_fail(open_host != NULL, FALSE);
+    g_return_val_if_fail(open_host->socket == NULL, FALSE);
+
+    open_host->proxy = get_proxy(&open_host->error);
+    if (open_host->proxy) {
+        g_resolver_lookup_by_name_async(g_resolver_get_default(),
+                                        spice_proxy_get_hostname(open_host->proxy),
+                                        open_host->cancellable,
+                                        proxy_lookup_ready, open_host);
+    } else if (open_host->error != NULL) {
+        coroutine_yieldto(open_host->from, NULL);
+        return FALSE;
+    } else
+        open_host_connectable_connect(open_host,
+                                      g_network_address_new(s->host, open_host->port));
+
+    SPICE_DEBUG("open host %s:%d", s->host, open_host->port);
+    if (open_host->proxy != NULL)
+        SPICE_DEBUG("(with proxy %p)", open_host->proxy);
 
-    return address;
+    return FALSE;
 }
 
 /* coroutine context */
@@ -1572,47 +1637,33 @@  GSocket* spice_session_channel_open_host(SpiceSession *session, SpiceChannel *ch
                                          gboolean use_tls)
 {
     SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
-    GSocketClient *client = NULL;
-    GSocketConnection *connection = NULL;
-    GSocketAddress *address = NULL;
-    GSocketConnectable *connectable = NULL;
-    GSocket *socket = NULL;
-    GError *error = NULL;
-    int port;
+    spice_open_host open_host = { 0, };
 
     if ((use_tls && !s->tls_port) || (!use_tls && !s->port))
         return NULL;
 
-    port = atoi(use_tls ? s->tls_port : s->port);
-
-    /* FIXME: make all of this async again */
-    client = g_socket_client_new();
-    address = get_proxy_address(session, port, &error);
-    if (error != NULL) {
-        g_critical("%s", error->message);
-        goto end;
-    }
+    open_host.from = coroutine_self();
+    open_host.session = session;
+    open_host.channel = channel;
+    open_host.port = atoi(use_tls ? s->tls_port : s->port);
+    g_idle_add(open_host_idle_cb, &open_host);
 
-    if (address != NULL)
-        connectable = G_SOCKET_CONNECTABLE(address);
-    else
-        connectable = g_network_address_new(s->host, port);
+    /* switch to main loop and wait for connection */
+    coroutine_yield(NULL);
+    if (open_host.error != NULL) {
+        g_return_val_if_fail(open_host.socket == NULL, NULL);
 
-    connection = g_socket_client_connect(client, connectable, NULL, &error);
-    if (connection == NULL)
-        goto end;
-
-    socket = g_socket_connection_get_socket(connection);
-    g_socket_set_blocking(socket, FALSE);
-    g_socket_set_keepalive(socket, TRUE);
-    g_object_ref(socket);
+        g_warning("%s", open_host.error->message);
+        g_clear_error(&open_host.error);
+    } else {
+        g_return_val_if_fail(open_host.socket != NULL, NULL);
 
-end:
-    g_clear_object(&connection);
-    g_clear_object(&connectable);
-    g_clear_object(&client);
+        g_socket_set_blocking(open_host.socket, FALSE);
+        g_socket_set_keepalive(open_host.socket, TRUE);
+    }
 
-    return socket;
+    g_clear_object(&open_host.proxy);
+    return open_host.socket;
 }
 
 

Comments

On Fri, Aug 24, 2012 at 03:58:19PM +0200, Marc-André Lureau wrote:
> The previous patches (introducing GSocketClient & GProxy usage) turned
> the code into only sync calls.

This would be much nicer to review if the GProxy changes were done
separately I think, or folded into patch 4/6


Christophe

> 
> By using async() variants of resolve & connect functions, make the
> open host connection asynchronous again.
> ---
>  gtk/spice-session.c | 195 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 123 insertions(+), 72 deletions(-)
> 
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index ae70b73..1688c58 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -28,6 +28,7 @@
>  #include "gio-coroutine.h"
>  #include "glib-compat.h"
>  #include "wocky-http-proxy.h"
> +#include "spice-proxy.h"
>  
>  struct channel {
>      SpiceChannel      *channel;
> @@ -1511,59 +1512,123 @@ gboolean spice_session_has_channel_type(SpiceSession *session, gint type)
>  /* ------------------------------------------------------------------ */
>  /* private functions                                                  */
>  
> -static GSocketAddress* get_proxy_address(SpiceSession *session, guint port, GError **error)
> +static SpiceProxy* get_proxy(GError **error)
>  {
> -    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> -    GSocketAddress *address = NULL;
> -    const gchar *proxy = g_getenv("SPICE_PROXY");
> -    GList *addresses = NULL, *it;
> +    SpiceProxy *proxy;
>  
> -    if (proxy == NULL)
> +    const gchar *proxy_env = g_getenv("SPICE_PROXY");
> +    if (proxy_env == NULL || strlen(proxy_env) == 0)
>          return NULL;
>  
> -    /* FIXME: use GUri when it is ready... */
> -    if (g_ascii_strncasecmp("http://", proxy, 7) == 0)
> -        proxy += 7;
> -
> -    gchar **proxyv = g_strsplit(proxy, ":", 0);
> -    guint16 pport = 3128;
> -    const gchar *proxy_host = NULL, *proxy_port = NULL;
> -
> -    proxy_host = proxyv[0];
> -    if (proxy_host != NULL)
> -        proxy_port = proxyv[1];
> -
> -    if (proxy_port != NULL) {
> -        char *endptr;
> -        pport = strtoul(proxy_port, &endptr, 10);
> -        if (*endptr != '\0') {
> -            g_set_error(error, SPICE_CLIENT_ERROR, SPICE_CLIENT_ERROR_FAILED,
> -                        "Invalid proxy port: %s", proxy_port);
> -            goto end;
> -        }
> -    }
> +    proxy = spice_proxy_new();
> +    if (!spice_proxy_parse(proxy, proxy_env, error))
> +        g_clear_object(&proxy);
> +
> +    return proxy;
> +}
> +
> +typedef struct spice_open_host spice_open_host;
>  
> -    addresses = g_resolver_lookup_by_name(g_resolver_get_default(),
> -                                          proxy_host, NULL, error);
> -    if (*error != NULL)
> +struct spice_open_host {
> +    struct coroutine *from;
> +    SpiceSession *session;
> +    SpiceChannel *channel;
> +    SpiceProxy *proxy;
> +    int port;
> +    GCancellable *cancellable;
> +    GError *error;
> +    GSocket *socket;
> +};
> +
> +static void socket_client_connect_ready(GObject *source_object, GAsyncResult *result,
> +                                        gpointer data)
> +{
> +    GSocketClient *client = G_SOCKET_CLIENT(source_object);
> +    spice_open_host *open_host = data;
> +    GSocketConnection *connection = NULL;
> +
> +    SPICE_DEBUG("connect ready");
> +    connection = g_socket_client_connect_finish(client, result, &open_host->error);
> +    if (connection == NULL)
>          goto end;
>  
> -    /* FIXME: iterate over all addresses
> -     * gproxy makes it quite unconvenient to deal with hostname,
> -     * it would make sense to do it once earlier?
> -     */
> +    open_host->socket = g_socket_connection_get_socket(connection);
> +    g_object_ref(open_host->socket);
> +
> +end:
> +    g_object_unref(connection);
> +    g_object_unref(client);
> +
> +    coroutine_yieldto(open_host->from, NULL);
> +}
> +
> +/* main context */
> +static void open_host_connectable_connect(spice_open_host *open_host, GSocketConnectable *connectable)
> +{
> +    GSocketClient *client;
> +
> +    SPICE_DEBUG("connecting %p...", open_host);
> +    client = g_socket_client_new();
> +    g_socket_client_connect_async(client, connectable, open_host->cancellable,
> +                                  socket_client_connect_ready, open_host);
> +}
> +
> +/* main context */
> +static void proxy_lookup_ready(GObject *source_object, GAsyncResult *result,
> +                               gpointer data)
> +{
> +    spice_open_host *open_host = data;
> +    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(open_host->session);
> +    GList *addresses = NULL, *it;
> +    GSocketAddress *address;
> +
> +    SPICE_DEBUG("proxy lookup ready");
> +    addresses = g_resolver_lookup_by_name_finish(G_RESOLVER(source_object),
> +                                                 result, &open_host->error);
> +    if (addresses == NULL || open_host->error) {
> +        coroutine_yieldto(open_host->from, NULL);
> +        return;
> +    }
> +
>      for (it = addresses; it != NULL; it = it->next) {
> -        address = g_proxy_address_new(G_INET_ADDRESS(it->data), pport, "http",
> -                                      s->host, port, NULL, NULL);
> +        address = g_proxy_address_new(G_INET_ADDRESS(it->data),
> +                                      spice_proxy_get_port(open_host->proxy), "http",
> +                                      s->host, open_host->port, NULL, NULL);
>          if (address != NULL)
>              break;
>      }
>  
> -end:
> +    open_host_connectable_connect(open_host, G_SOCKET_CONNECTABLE(address));
>      g_resolver_free_addresses(addresses);
> -    g_strfreev(proxyv);
> +}
> +
> +/* main context */
> +static gboolean open_host_idle_cb(gpointer data)
> +{
> +    spice_open_host *open_host = data;
> +    SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(open_host->session);
> +
> +    g_return_val_if_fail(open_host != NULL, FALSE);
> +    g_return_val_if_fail(open_host->socket == NULL, FALSE);
> +
> +    open_host->proxy = get_proxy(&open_host->error);
> +    if (open_host->proxy) {
> +        g_resolver_lookup_by_name_async(g_resolver_get_default(),
> +                                        spice_proxy_get_hostname(open_host->proxy),
> +                                        open_host->cancellable,
> +                                        proxy_lookup_ready, open_host);
> +    } else if (open_host->error != NULL) {
> +        coroutine_yieldto(open_host->from, NULL);
> +        return FALSE;
> +    } else
> +        open_host_connectable_connect(open_host,
> +                                      g_network_address_new(s->host, open_host->port));
> +
> +    SPICE_DEBUG("open host %s:%d", s->host, open_host->port);
> +    if (open_host->proxy != NULL)
> +        SPICE_DEBUG("(with proxy %p)", open_host->proxy);
>  
> -    return address;
> +    return FALSE;
>  }
>  
>  /* coroutine context */
> @@ -1572,47 +1637,33 @@ GSocket* spice_session_channel_open_host(SpiceSession *session, SpiceChannel *ch
>                                           gboolean use_tls)
>  {
>      SpiceSessionPrivate *s = SPICE_SESSION_GET_PRIVATE(session);
> -    GSocketClient *client = NULL;
> -    GSocketConnection *connection = NULL;
> -    GSocketAddress *address = NULL;
> -    GSocketConnectable *connectable = NULL;
> -    GSocket *socket = NULL;
> -    GError *error = NULL;
> -    int port;
> +    spice_open_host open_host = { 0, };
>  
>      if ((use_tls && !s->tls_port) || (!use_tls && !s->port))
>          return NULL;
>  
> -    port = atoi(use_tls ? s->tls_port : s->port);
> -
> -    /* FIXME: make all of this async again */
> -    client = g_socket_client_new();
> -    address = get_proxy_address(session, port, &error);
> -    if (error != NULL) {
> -        g_critical("%s", error->message);
> -        goto end;
> -    }
> +    open_host.from = coroutine_self();
> +    open_host.session = session;
> +    open_host.channel = channel;
> +    open_host.port = atoi(use_tls ? s->tls_port : s->port);
> +    g_idle_add(open_host_idle_cb, &open_host);
>  
> -    if (address != NULL)
> -        connectable = G_SOCKET_CONNECTABLE(address);
> -    else
> -        connectable = g_network_address_new(s->host, port);
> +    /* switch to main loop and wait for connection */
> +    coroutine_yield(NULL);
> +    if (open_host.error != NULL) {
> +        g_return_val_if_fail(open_host.socket == NULL, NULL);
>  
> -    connection = g_socket_client_connect(client, connectable, NULL, &error);
> -    if (connection == NULL)
> -        goto end;
> -
> -    socket = g_socket_connection_get_socket(connection);
> -    g_socket_set_blocking(socket, FALSE);
> -    g_socket_set_keepalive(socket, TRUE);
> -    g_object_ref(socket);
> +        g_warning("%s", open_host.error->message);
> +        g_clear_error(&open_host.error);
> +    } else {
> +        g_return_val_if_fail(open_host.socket != NULL, NULL);
>  
> -end:
> -    g_clear_object(&connection);
> -    g_clear_object(&connectable);
> -    g_clear_object(&client);
> +        g_socket_set_blocking(open_host.socket, FALSE);
> +        g_socket_set_keepalive(open_host.socket, TRUE);
> +    }
>  
> -    return socket;
> +    g_clear_object(&open_host.proxy);
> +    return open_host.socket;
>  }
>  
>  
> -- 
> 1.7.11.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Thu, Oct 18, 2012 at 6:08 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
> On Fri, Aug 24, 2012 at 03:58:19PM +0200, Marc-André Lureau wrote:
>> The previous patches (introducing GSocketClient & GProxy usage) turned
>> the code into only sync calls.
>
> This would be much nicer to review if the GProxy changes were done
> separately I think, or folded into patch 4/6
>

Can you be more precise? from a quick glance, I failed to see what you
are talking about, but I have not looked at the code for a long time.
You probably have it in mind more than me :)

thanks
On Thu, Oct 18, 2012 at 06:11:58PM +0200, Marc-André Lureau wrote:
> On Thu, Oct 18, 2012 at 6:08 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > On Fri, Aug 24, 2012 at 03:58:19PM +0200, Marc-André Lureau wrote:
> >> The previous patches (introducing GSocketClient & GProxy usage) turned
> >> the code into only sync calls.
> >
> > This would be much nicer to review if the GProxy changes were done
> > separately I think, or folded into patch 4/6
> >
> 
> Can you be more precise? from a quick glance, I failed to see what you
> are talking about, but I have not looked at the code for a long time.
> You probably have it in mind more than me :)

4/6 adds the http://hostname:port parsing to spice-session.c, 5/6
introduces SpiceProxy which contains this parsing code as well, 6/6
switches spice-session.c to using SpiceProxy and also makes things async.

It would be nicer to add SpiceProxy, then make spice-session.c handle proxy
synchronously using SpiceProxy, and then making things async.

Christophe