[Spice-devel] [PATCH spice-gtk 2/2] Add SpiceSession:proxy

[Christophe Fergeau]

Hey,

On Mon, Jan 28, 2013 at 09:19:33PM +0100, Marc-André Lureau wrote:
> Add a session property to set proxy setting, since it is racy to rely
> on setenv(). Also doing so would override system environment, which
> will modify other session too sharing the same process.
> ---
>  gtk/spice-proxy.c   | 10 ++++++++++
>  gtk/spice-proxy.h   |  1 +
>  gtk/spice-session.c | 54 ++++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/gtk/spice-proxy.c b/gtk/spice-proxy.c
> index 97c3a6b..2182f33 100644
> --- a/gtk/spice-proxy.c
> +++ b/gtk/spice-proxy.c
> @@ -234,3 +234,13 @@ static void spice_proxy_class_init(SpiceProxyClass *klass)
>                                                         G_PARAM_STATIC_STRINGS |
>                                                         G_PARAM_READWRITE));
>  }
> +
> +gchar* spice_proxy_to_string(SpiceProxy* self)
> +{
> +    SpiceProxyPrivate *p;
> +
> +    g_return_val_if_fail(SPICE_IS_PROXY(self), NULL);
> +    p = self->priv;
> +
> +    return g_strdup_printf("%s://%s:%u", p->protocol, p->hostname, p->port);
> +}
> diff --git a/gtk/spice-proxy.h b/gtk/spice-proxy.h
> index c780931..1e7b6d7 100644
> --- a/gtk/spice-proxy.h
> +++ b/gtk/spice-proxy.h
> @@ -53,6 +53,7 @@ const gchar* spice_proxy_get_hostname(SpiceProxy* self);
>  void spice_proxy_set_hostname(SpiceProxy* self, const gchar* value);
>  guint spice_proxy_get_port(SpiceProxy* self);
>  void spice_proxy_set_port(SpiceProxy* self, guint port);
> +gchar *spice_proxy_to_string(SpiceProxy* self);
>  
>  G_END_DECLS
>  
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index 5beb6a1..f55f905 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -106,6 +106,7 @@ enum {
>      PROP_UUID,
>      PROP_NAME,
>      PROP_CA,
> +    PROP_PROXY
>  };
>  
>  /* signals */
> @@ -118,24 +119,25 @@ enum {
>  static guint signals[SPICE_SESSION_LAST_SIGNAL];
>  
>  
> -static SpiceProxy* get_proxy(void)
> +static void update_proxy(SpiceSession *self, const gchar *str)
>  {
> +    SpiceSessionPrivate *s = self->priv;
>      GError *error = NULL;
> -    SpiceProxy *proxy;
>  
> -    const gchar *proxy_env = g_getenv("SPICE_PROXY");
> -    if (proxy_env == NULL || strlen(proxy_env) == 0)
> -        return NULL;
> +    g_clear_object(&s->proxy);
> +
> +    if (str == NULL)
> +        str = g_getenv("SPICE_PROXY");

Ah good, I initially thought the patch was dropping support for the
environment variable.

> +    if (str == NULL || strlen(str) == 0)

I have a slight preference for *str != '\0' instead of strlen(str), but
that's mostly a matter of taste here as this is nowhere performance
critical.

> +        return;
>  
> -    proxy = spice_proxy_new();
> -    if (!spice_proxy_parse(proxy, proxy_env, &error))
> -        g_clear_object(&proxy);
> +    s->proxy = spice_proxy_new();
> +    if (!spice_proxy_parse(s->proxy, str, &error))
> +        g_clear_object(&s->proxy);
>      if (error) {
>          g_warning ("%s", error->message);
>          g_clear_error (&error);
>      }
> -
> -    return proxy;

update_proxy is setting the proxy to NULL on invalid input, it wouldn't be
much harder to keep the old proxy setting when it's passed an invalid 'str'
parameter. I *think* this behaviour would be a bit better, but no strong
feeling either way.

>  }
>  
>  static void spice_session_init(SpiceSession *session)
> @@ -167,7 +169,7 @@ static void spice_session_init(SpiceSession *session)
>      cache_init(&s->images, "image");
>      cache_init(&s->palettes, "palette");
>      s->glz_window = glz_decoder_window_new();
> -    s->proxy = get_proxy();
> +    update_proxy(session, NULL);

This could also be achieved without explicitly calling update_proxy() by
marking PROP_PROXY as _CONSTRUCT. I'm fine with the explicit call as well.

>  }
>  
>  static void
> @@ -513,6 +515,9 @@ static void spice_session_get_property(GObject    *gobject,
>      case PROP_UUID:
>          g_value_set_pointer(value, s->uuid);
>  	break;
> +    case PROP_PROXY:
> +        g_value_take_string(value, spice_proxy_to_string(s->proxy));
> +	break;
>      default:
>  	G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
>  	break;
> @@ -628,6 +633,9 @@ static void spice_session_set_property(GObject      *gobject,
>          g_clear_pointer(&s->ca, g_byte_array_unref);
>          s->ca = g_value_dup_boxed(value);
>          break;
> +    case PROP_PROXY:
> +        update_proxy(session, g_value_get_string(value));
> +        break;
>      default:
>          G_OBJECT_WARN_INVALID_PROPERTY_ID(gobject, prop_id, pspec);
>          break;
> @@ -1119,6 +1127,23 @@ static void spice_session_class_init(SpiceSessionClass *klass)
>                                G_PARAM_READABLE |
>                                G_PARAM_STATIC_STRINGS));
>  
> +    /**
> +     * SpiceSession:proxy:
> +     *
> +     * The proxy server to use when doing network connection.
> +     * of the form <![CDATA[ [protocol://]<host>[:port] ]]>
> +     *
> +     * Since: 0.17
> +     **/
> +    g_object_class_install_property
> +        (gobject_class, PROP_PROXY,
> +         g_param_spec_string("proxy",
> +                             "Proxy",
> +                             "The proxy server",

"URI to the proxy server" would be more descriptive.

Looks good apart from these small comments!

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130129/74acdf25/attachment.pgp>