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
Attachment:
pgpqLUdhjgVff.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel