Re: [Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-12 Thread Christophe Fergeau 
On Tue, Feb 11, 2014 at 06:15:15PM +0100, Marc-André Lureau wrote:
 I am not fond of super-extra-small changes, I tend to make things that
 belong together in the same patch. This one could be splitted in 5
 patches or more, I don't see the point. All I want is a reasonable set
 of related changes that don't break things.

Well, this makes review more complicated. I was focused on reviewing a
change parsing user and password, so I tried to interpret this change as
something helping with that. When it did not seem useful for parsing
username and password, I was still stuck with am I missing something, or
is it just a parsing improvement?
If this small change were to be buggy in some corner cases a few years from
now, we'd be misled again if git bisect points us at a commit adding
user/password parsing rather than at a small commit doing some minor
cleanup. And we'll also won't know anything about the intent of this change
(was it a small and obvious cleanup? was it required for some reason? what
was that reason?).
So while it may make things more convenient now for the person doing the
commit, it's in my opinion (well not just me) better to avoid having silent
changes in commits.

 So should I split it or you agree it's fine as part of parsing changes?

I initially was planning to tell you to just go with it, and now I just
convinced myself that it's better to split it /o\

Christophe


pgpeTDDg3ZTkj.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-12 Thread Marc-André Lureau 
On Wed, Feb 12, 2014 at 10:39 AM, Christophe Fergeau
cferg...@redhat.com wrote:
 On Tue, Feb 11, 2014 at 06:15:15PM +0100, Marc-André Lureau wrote:
 I am not fond of super-extra-small changes, I tend to make things that
 belong together in the same patch. This one could be splitted in 5
 patches or more, I don't see the point. All I want is a reasonable set
 of related changes that don't break things.

 Well, this makes review more complicated. I was focused on reviewing a
 change parsing user and password, so I tried to interpret this change as
 something helping with that. When it did not seem useful for parsing
 username and password, I was still stuck with am I missing something, or
 is it just a parsing improvement?

ok

 If this small change were to be buggy in some corner cases a few years from
 now, we'd be misled again if git bisect points us at a commit adding
 user/password parsing rather than at a small commit doing some minor
 cleanup. And we'll also won't know anything about the intent of this change
 (was it a small and obvious cleanup? was it required for some reason? what
 was that reason?).

In general, I agree, but I consider this to be simple, non-essential
and hopefully replacable in a near future with a real GUri parser (or
equivalent, like SoupUri)

 So while it may make things more convenient now for the person doing the
 commit, it's in my opinion (well not just me) better to avoid having silent
 changes in commits.
 So should I split it or you agree it's fine as part of parsing changes?

 I initially was planning to tell you to just go with it, and now I just
 convinced myself that it's better to split it /o\

I'll split this change off then.



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-11 Thread Christophe Fergeau 
On Mon, Feb 03, 2014 at 07:02:34PM +0100, Marc-André Lureau wrote:
 From: Marc-André Lureau marcandre.lur...@redhat.com
 
 Someday this ought to be GURI.. or SoupUri?
 ---
  gtk/spice-proxy.c   | 84 
 -
  gtk/spice-proxy.h   |  4 +++
  gtk/spice-session.c |  2 +-
  3 files changed, 88 insertions(+), 2 deletions(-)
 
 diff --git a/gtk/spice-proxy.c b/gtk/spice-proxy.c
 index bc4037e..834aa10 100644
 --- a/gtk/spice-proxy.c
 +++ b/gtk/spice-proxy.c
 @@ -19,6 +19,7 @@
  #include stdlib.h
  #include string.h
  
 +#include glib-compat.h
  #include spice-client.h
  #include spice-proxy.h
  
 @@ -26,6 +27,8 @@ struct _SpiceProxyPrivate {
  gchar *protocol;
  gchar *hostname;
  guint port;
 +gchar *user;
 +gchar *password;
  };
  
  #define SPICE_PROXY_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o), 
 SPICE_TYPE_PROXY, SpiceProxyPrivate))
 @@ -35,6 +38,8 @@ G_DEFINE_TYPE(SpiceProxy, spice_proxy, G_TYPE_OBJECT);
  enum  {
  SPICE_PROXY_DUMMY_PROPERTY,
  SPICE_PROXY_PROTOCOL,
 +SPICE_PROXY_USER,
 +SPICE_PROXY_PASSWORD,
  SPICE_PROXY_HOSTNAME,
  SPICE_PROXY_PORT
  };
 @@ -74,7 +79,18 @@ gboolean spice_proxy_parse(SpiceProxy *self, const gchar 
 *proxyuri, GError **err
  spice_proxy_set_protocol(self, http);
  spice_proxy_set_port(self, 3128);
  
 -gchar **proxyv = g_strsplit(uri, :, 0);
 +gchar *saveptr, *auth = strtok_r(uri, @, saveptr);

gchar *saveptr;
gchar *auth = strtok_r(uri, @, saveprtr);

 +if (saveptr  *saveptr) {

I don't think anything can be assumed about the value of saveptr.
if (auth != NULL) { ... } ?

 +gchar *saveptr2;
 +const gchar *user = strtok_r(auth, :, saveptr2);
 +const gchar *pass = strtok_r(NULL, :, saveptr2);
 +spice_proxy_set_user(self, user);
 +spice_proxy_set_password(self, pass);
 +g_debug(user: %s pass: %s, user, pass);
 +uri = saveptr;

Same comment about using the value of saveptr.

 +}
 +
 +gchar **proxyv = g_strsplit(uri, :, 2);

This does not seem to be strictly related to this change.

  const gchar *proxy_port = NULL;
  
  if (proxyv[0] == NULL || strlen(proxyv[0]) == 0) {
 @@ -172,6 +188,12 @@ static void spice_proxy_get_property(GObject *object, 
 guint property_id,
  case SPICE_PROXY_PORT:
  g_value_set_uint(value, spice_proxy_get_port(self));
  break;
 +case SPICE_PROXY_USER:
 +g_value_set_string(value, spice_proxy_get_user(self));
 +break;
 +case SPICE_PROXY_PASSWORD:
 +g_value_set_string(value, spice_proxy_get_password(self));
 +break;
  default:
  G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
  break;
 @@ -192,6 +214,12 @@ static void spice_proxy_set_property(GObject *object, 
 guint property_id,
  case SPICE_PROXY_HOSTNAME:
  spice_proxy_set_hostname(self, g_value_get_string(value));
  break;
 +case SPICE_PROXY_USER:
 +spice_proxy_set_user(self, g_value_get_string(value));
 +break;
 +case SPICE_PROXY_PASSWORD:
 +spice_proxy_set_password(self, g_value_get_string(value));
 +break;
  case SPICE_PROXY_PORT:
  spice_proxy_set_port(self, g_value_get_uint(value));
  break;
 @@ -253,6 +281,24 @@ static void spice_proxy_class_init(SpiceProxyClass 
 *klass)
 0, G_MAXUINT, 0,
 
 G_PARAM_STATIC_STRINGS |
 G_PARAM_READWRITE));
 +
 +g_object_class_install_property(G_OBJECT_CLASS (klass),
 +SPICE_PROXY_USER,
 +g_param_spec_string (user,
 + user,
 + user,
 + NULL,
 + 
 G_PARAM_STATIC_STRINGS |
 + G_PARAM_READWRITE));
 +
 +g_object_class_install_property(G_OBJECT_CLASS (klass),
 +SPICE_PROXY_PASSWORD,
 +g_param_spec_string (password,
 + password,
 + password,
 + NULL,
 + 
 G_PARAM_STATIC_STRINGS |
 + G_PARAM_READWRITE));
  }
  
  G_GNUC_INTERNAL
 @@ -268,3 +314,39 @@ gchar* spice_proxy_to_string(SpiceProxy* self)
  
  return g_strdup_printf(%s://%s:%u, p-protocol, p-hostname, p-port);
  }
 +
 +G_GNUC_INTERNAL
 +const gchar* spice_proxy_get_user(SpiceProxy

Re: [Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-11 Thread Marc-André Lureau 


- Original Message -
 On Mon, Feb 03, 2014 at 07:02:34PM +0100, Marc-André Lureau wrote:
  From: Marc-André Lureau marcandre.lur...@redhat.com
  
  Someday this ought to be GURI.. or SoupUri?
  ---
   gtk/spice-proxy.c   | 84
   -
   gtk/spice-proxy.h   |  4 +++
   gtk/spice-session.c |  2 +-
   3 files changed, 88 insertions(+), 2 deletions(-)
  
  diff --git a/gtk/spice-proxy.c b/gtk/spice-proxy.c
  index bc4037e..834aa10 100644
  --- a/gtk/spice-proxy.c
  +++ b/gtk/spice-proxy.c
  @@ -19,6 +19,7 @@
   #include stdlib.h
   #include string.h
   
  +#include glib-compat.h
   #include spice-client.h
   #include spice-proxy.h
   
  @@ -26,6 +27,8 @@ struct _SpiceProxyPrivate {
   gchar *protocol;
   gchar *hostname;
   guint port;
  +gchar *user;
  +gchar *password;
   };
   
   #define SPICE_PROXY_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o),
   SPICE_TYPE_PROXY, SpiceProxyPrivate))
  @@ -35,6 +38,8 @@ G_DEFINE_TYPE(SpiceProxy, spice_proxy, G_TYPE_OBJECT);
   enum  {
   SPICE_PROXY_DUMMY_PROPERTY,
   SPICE_PROXY_PROTOCOL,
  +SPICE_PROXY_USER,
  +SPICE_PROXY_PASSWORD,
   SPICE_PROXY_HOSTNAME,
   SPICE_PROXY_PORT
   };
  @@ -74,7 +79,18 @@ gboolean spice_proxy_parse(SpiceProxy *self, const gchar
  *proxyuri, GError **err
   spice_proxy_set_protocol(self, http);
   spice_proxy_set_port(self, 3128);
   
  -gchar **proxyv = g_strsplit(uri, :, 0);
  +gchar *saveptr, *auth = strtok_r(uri, @, saveptr);
 
 gchar *saveptr;
 gchar *auth = strtok_r(uri, @, saveprtr);
 

ok

  +if (saveptr  *saveptr) {
 
 I don't think anything can be assumed about the value of saveptr.

ok

 if (auth != NULL) { ... } ?
 
  +gchar *saveptr2;
  +const gchar *user = strtok_r(auth, :, saveptr2);
  +const gchar *pass = strtok_r(NULL, :, saveptr2);
  +spice_proxy_set_user(self, user);
  +spice_proxy_set_password(self, pass);
  +g_debug(user: %s pass: %s, user, pass);
  +uri = saveptr;
 
 Same comment about using the value of saveptr.
 
  +}
  +
  +gchar **proxyv = g_strsplit(uri, :, 2);
 
 This does not seem to be strictly related to this change.

You mean 0 - 2.. ? ok...

   const gchar *proxy_port = NULL;
   
   if (proxyv[0] == NULL || strlen(proxyv[0]) == 0) {
  @@ -172,6 +188,12 @@ static void spice_proxy_get_property(GObject *object,
  guint property_id,
   case SPICE_PROXY_PORT:
   g_value_set_uint(value, spice_proxy_get_port(self));
   break;
  +case SPICE_PROXY_USER:
  +g_value_set_string(value, spice_proxy_get_user(self));
  +break;
  +case SPICE_PROXY_PASSWORD:
  +g_value_set_string(value, spice_proxy_get_password(self));
  +break;
   default:
   G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
   break;
  @@ -192,6 +214,12 @@ static void spice_proxy_set_property(GObject *object,
  guint property_id,
   case SPICE_PROXY_HOSTNAME:
   spice_proxy_set_hostname(self, g_value_get_string(value));
   break;
  +case SPICE_PROXY_USER:
  +spice_proxy_set_user(self, g_value_get_string(value));
  +break;
  +case SPICE_PROXY_PASSWORD:
  +spice_proxy_set_password(self, g_value_get_string(value));
  +break;
   case SPICE_PROXY_PORT:
   spice_proxy_set_port(self, g_value_get_uint(value));
   break;
  @@ -253,6 +281,24 @@ static void spice_proxy_class_init(SpiceProxyClass
  *klass)
  0, G_MAXUINT, 0,
  
  G_PARAM_STATIC_STRINGS
  |
  G_PARAM_READWRITE));
  +
  +g_object_class_install_property(G_OBJECT_CLASS (klass),
  +SPICE_PROXY_USER,
  +g_param_spec_string (user,
  + user,
  + user,
  + NULL,
  +
  G_PARAM_STATIC_STRINGS
  |
  +
  G_PARAM_READWRITE));
  +
  +g_object_class_install_property(G_OBJECT_CLASS (klass),
  +SPICE_PROXY_PASSWORD,
  +g_param_spec_string (password,
  + password,
  + password,
  + NULL,
  +
  G_PARAM_STATIC_STRINGS
  |
  +
  G_PARAM_READWRITE));
   }
   
   G_GNUC_INTERNAL
  @@ -268,3 +314,39 @@ gchar* spice_proxy_to_string(SpiceProxy* self)
   
   return g_strdup_printf(%s://%s:%u, p-protocol, p-hostname,
   p-port);
   }
  +
  +G_GNUC_INTERNAL

Re: [Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-11 Thread Christophe Fergeau 
On Tue, Feb 11, 2014 at 12:00:58PM -0500, Marc-André Lureau wrote:
   +}
   +
   +gchar **proxyv = g_strsplit(uri, :, 2);
  
  This does not seem to be strictly related to this change.
 
 You mean 0 - 2.. ? ok...

(not saying this should be split out, ..., just mentioning it in case this
belongs in another patch or was not meant to be there).

Christophe


pgpUoBHvknWAN.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-11 Thread Marc-André Lureau 
On Tue, Feb 11, 2014 at 6:06 PM, Christophe Fergeau cferg...@redhat.com wrote:
 On Tue, Feb 11, 2014 at 12:00:58PM -0500, Marc-André Lureau wrote:
   +}
   +
   +gchar **proxyv = g_strsplit(uri, :, 2);
 
  This does not seem to be strictly related to this change.

 You mean 0 - 2.. ? ok...

 (not saying this should be split out, ..., just mentioning it in case this
 belongs in another patch or was not meant to be there).


It's a slight parsing improvements, just making sure we don't split
more than 2 pieces.

I am not fond of super-extra-small changes, I tend to make things that
belong together in the same patch. This one could be splitted in 5
patches or more, I don't see the point. All I want is a reasonable set
of related changes that don't break things. So should I split it or
you agree it's fine as part of parsing changes?




-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk 03/13] spice-proxy: parse user and pass

2014-02-03 Thread Marc-André Lureau 
From: Marc-André Lureau marcandre.lur...@redhat.com

Someday this ought to be GURI.. or SoupUri?
---
 gtk/spice-proxy.c   | 84 -
 gtk/spice-proxy.h   |  4 +++
 gtk/spice-session.c |  2 +-
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/gtk/spice-proxy.c b/gtk/spice-proxy.c
index bc4037e..834aa10 100644
--- a/gtk/spice-proxy.c
+++ b/gtk/spice-proxy.c
@@ -19,6 +19,7 @@
 #include stdlib.h
 #include string.h
 
+#include glib-compat.h
 #include spice-client.h
 #include spice-proxy.h
 
@@ -26,6 +27,8 @@ struct _SpiceProxyPrivate {
 gchar *protocol;
 gchar *hostname;
 guint port;
+gchar *user;
+gchar *password;
 };
 
 #define SPICE_PROXY_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o), 
SPICE_TYPE_PROXY, SpiceProxyPrivate))
@@ -35,6 +38,8 @@ G_DEFINE_TYPE(SpiceProxy, spice_proxy, G_TYPE_OBJECT);
 enum  {
 SPICE_PROXY_DUMMY_PROPERTY,
 SPICE_PROXY_PROTOCOL,
+SPICE_PROXY_USER,
+SPICE_PROXY_PASSWORD,
 SPICE_PROXY_HOSTNAME,
 SPICE_PROXY_PORT
 };
@@ -74,7 +79,18 @@ gboolean spice_proxy_parse(SpiceProxy *self, const gchar 
*proxyuri, GError **err
 spice_proxy_set_protocol(self, http);
 spice_proxy_set_port(self, 3128);
 
-gchar **proxyv = g_strsplit(uri, :, 0);
+gchar *saveptr, *auth = strtok_r(uri, @, saveptr);
+if (saveptr  *saveptr) {
+gchar *saveptr2;
+const gchar *user = strtok_r(auth, :, saveptr2);
+const gchar *pass = strtok_r(NULL, :, saveptr2);
+spice_proxy_set_user(self, user);
+spice_proxy_set_password(self, pass);
+g_debug(user: %s pass: %s, user, pass);
+uri = saveptr;
+}
+
+gchar **proxyv = g_strsplit(uri, :, 2);
 const gchar *proxy_port = NULL;
 
 if (proxyv[0] == NULL || strlen(proxyv[0]) == 0) {
@@ -172,6 +188,12 @@ static void spice_proxy_get_property(GObject *object, 
guint property_id,
 case SPICE_PROXY_PORT:
 g_value_set_uint(value, spice_proxy_get_port(self));
 break;
+case SPICE_PROXY_USER:
+g_value_set_string(value, spice_proxy_get_user(self));
+break;
+case SPICE_PROXY_PASSWORD:
+g_value_set_string(value, spice_proxy_get_password(self));
+break;
 default:
 G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
 break;
@@ -192,6 +214,12 @@ static void spice_proxy_set_property(GObject *object, 
guint property_id,
 case SPICE_PROXY_HOSTNAME:
 spice_proxy_set_hostname(self, g_value_get_string(value));
 break;
+case SPICE_PROXY_USER:
+spice_proxy_set_user(self, g_value_get_string(value));
+break;
+case SPICE_PROXY_PASSWORD:
+spice_proxy_set_password(self, g_value_get_string(value));
+break;
 case SPICE_PROXY_PORT:
 spice_proxy_set_port(self, g_value_get_uint(value));
 break;
@@ -253,6 +281,24 @@ static void spice_proxy_class_init(SpiceProxyClass *klass)
0, G_MAXUINT, 0,
G_PARAM_STATIC_STRINGS |
G_PARAM_READWRITE));
+
+g_object_class_install_property(G_OBJECT_CLASS (klass),
+SPICE_PROXY_USER,
+g_param_spec_string (user,
+ user,
+ user,
+ NULL,
+ 
G_PARAM_STATIC_STRINGS |
+ G_PARAM_READWRITE));
+
+g_object_class_install_property(G_OBJECT_CLASS (klass),
+SPICE_PROXY_PASSWORD,
+g_param_spec_string (password,
+ password,
+ password,
+ NULL,
+ 
G_PARAM_STATIC_STRINGS |
+ G_PARAM_READWRITE));
 }
 
 G_GNUC_INTERNAL
@@ -268,3 +314,39 @@ gchar* spice_proxy_to_string(SpiceProxy* self)
 
 return g_strdup_printf(%s://%s:%u, p-protocol, p-hostname, p-port);
 }
+
+G_GNUC_INTERNAL
+const gchar* spice_proxy_get_user(SpiceProxy *self)
+{
+g_return_val_if_fail(SPICE_IS_PROXY(self), NULL);
+return self-priv-user;
+}
+
+
+G_GNUC_INTERNAL
+void spice_proxy_set_user(SpiceProxy *self, const gchar *value)
+{
+g_return_if_fail(SPICE_IS_PROXY(self));
+
+g_free(self-priv-user);
+self-priv-user = g_strdup(value);
+g_object_notify((GObject *)self, user);
+}
+
+G_GNUC_INTERNAL
+const gchar* spice_proxy_get_password(SpiceProxy *self)
+{
+