[Spice-devel] [PATCH spice-gtk v4 4/8] spice-uri: Reset SpiceUri before parsing

[Victor Toso]

Hi,

On Tue, May 31, 2016 at 10:29:30AM +0200, Pavel Grunt wrote:
> Hi Victor,
> 
> On Tue, 2016-05-31 at 10:01 +0200, Victor Toso wrote:
> > Hi,
> > 
> > On Mon, May 30, 2016 at 05:46:46PM +0200, Pavel Grunt wrote:
> > > Avoid using old values after parsing a new uri.
> > > 
> > > Related: rhbz#1335239
> > > ---
> > >  src/spice-uri.c        | 17 +++++++++++++----
> > >  tests/test-spice-uri.c |  4 ++--
> > >  2 files changed, 15 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/spice-uri.c b/src/spice-uri.c
> > > index 3bdb502..9f793de 100644
> > > --- a/src/spice-uri.c
> > > +++ b/src/spice-uri.c
> > > @@ -97,6 +97,15 @@ SpiceURI* spice_uri_new(void)
> > >      return self;
> > >  }
> > >  
> > > +static void spice_uri_reset(SpiceURI *self)
> > > +{
> > > +    g_clear_pointer(&self->scheme, g_free);
> > > +    g_clear_pointer(&self->hostname, g_free);
> > > +    g_clear_pointer(&self->user, g_free);
> > > +    g_clear_pointer(&self->password, g_free);
> > > +    self->port = 0;
> > > +}
> > > +
> > >  G_GNUC_INTERNAL
> > >  gboolean spice_uri_parse(SpiceURI *self, const gchar *_uri, GError **error)
> > >  {
> > > @@ -105,6 +114,9 @@ gboolean spice_uri_parse(SpiceURI *self, const gchar
> > > *_uri, GError **error)
> > >      size_t len;
> > >  
> > >      g_return_val_if_fail(self != NULL, FALSE);
> > > +
> > > +    spice_uri_reset(self);
> > > +
> > >      g_return_val_if_fail(_uri != NULL, FALSE);
> > >  
> > >      uri = dup = g_strdup(_uri);
> > > @@ -333,10 +345,7 @@ static void spice_uri_finalize(GObject* obj)
> > >      SpiceURI *self;
> > >  
> > >      self = G_TYPE_CHECK_INSTANCE_CAST(obj, SPICE_TYPE_URI, SpiceURI);
> > > -    g_free(self->scheme);
> > > -    g_free(self->hostname);
> > > -    g_free(self->user);
> > > -    g_free(self->password);
> > > +    spice_uri_reset(self);
> > >  
> > >      G_OBJECT_CLASS (spice_uri_parent_class)->finalize (obj);
> > >  }
> > > diff --git a/tests/test-spice-uri.c b/tests/test-spice-uri.c
> > > index 00aeea7..e177723 100644
> > > --- a/tests/test-spice-uri.c
> > > +++ b/tests/test-spice-uri.c
> > > @@ -57,10 +57,10 @@ static void test_spice_uri_ipv4_bad(void)
> > >  static void test_spice_uri_ipv4_good(void)
> > >  {
> > >      const struct test_case valid_test_cases[] = {
> > > -        {"http://127.0.0.1/", "http", "127.0.0.1", 3128, NULL, NULL, NULL},
> > > +        {"http://user:password@host:80", "http", "host", 80, "user",
> > > "password", NULL},
> > > +        {"http://127.0.0.1/", "http", "127.0.0.1", 3128, NULL, NULL, NULL},
> > > /* reset user & password */
> > >          {"https://127.0.0.1", "https", "127.0.0.1", 3129, NULL, NULL,
> > > NULL},
> > >          {"127.0.0.1", "http", "127.0.0.1", 3128, NULL, NULL, NULL},
> > > -        {"http://user:password@host:80", "http", "host", 80, "user",
> > > "password", NULL},
> >
> > Why this changes here in the test-spice-uri? I see you moving a test and
> > including a comment... I would say that this belongs to the first patch?
> >
> This change is here to test that the SpiceURI is resetted. The first test case
> has hostname and user name set, without this patch "host" and "username" would
> survive to the other test case.

Aha! I got it now :)
So, previously you put the test case:

    {"http://user:password@host:80", "http", "host", 80, "user",
     "password", NULL}

In the end of tests as you know that "user" and "password" would be
re-used; but now that this is fixed you moved up to test it.

>
> I can fix the "problem" before creating tests, or change the comment ?

I would say that it would be more clear if you have separate test case
for it.

Cheers,
  toso

Reviewed-by: Victor Toso <victortoso at redhat.com>

>
> Pavel
>
>
> > >      };
> > >  
> > >      guint i;
> > > -- 
> > > 2.8.3
> > > 
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel