Created attachment 347162 [details] [review]
kerberos: Don't leak the name_owner

Created attachment 347163 [details] [review]
kerberos: Style fix

CCing Igor since he was suffering from a leak too.

Review of attachment 347162 [details] [review]:

eek

::: src/goabackend/goakerberosprovider.c
@@ +1334,1 @@
+  name_owner = g_dbus_proxy_get_name_owner (proxy);

nice find.

Thanks for the review, halfline. Pushed to master, gnome-3-22 and gnome-3-20.

mclasen spotted another leak in the same function.

Created attachment 347585 [details] [review]
kerberos: Don't leak the result

Completely untested. Will try it out tomorrow.

Created attachment 347638 [details] [review]
daemon: Don't rely on G_DISABLE_ASSERT being undefined

Minor problem - noticed while staring at the code.

Review of attachment 347585 [details] [review]:

hey, yea I was chatting with Matthias about this in meat space yesterday.

::: src/goabackend/goakerberosprovider.c
@@ +1360,3 @@
  out:
   g_clear_pointer (&iter, (GDestroyNotify) g_variant_iter_free);
+  g_clear_pointer (&result, (GDestroyNotify) g_variant_unref);

seems right to me. I almost wonder if this function should be gio ...

Review of attachment 347638 [details] [review]:

another eeker.

::: src/daemon/goadaemon.c
@@ +1508,3 @@
+
+  task_queued = G_TASK (g_queue_pop_head (self->ensure_credentials_queue));
+  g_assert (task == task_queued);

this is fine, but another way you could do it is..

g_assert (g_queue_peek_head (self->ensur...) == user_data);

task = G_TASK (g_queue_pop_head (self->ensur...));

Would get rid of the barely used extra GTask declaration then. But it's just a subjective style thing, really.  Also, I'm not sure which style I like better.  I'm just throwing it out there as an alternative.

(In reply to Ray Strode [halfline] from comment #9)
> ::: src/goabackend/goakerberosprovider.c
> @@ +1360,3 @@
>   out:
>    g_clear_pointer (&iter, (GDestroyNotify) g_variant_iter_free);
> +  g_clear_pointer (&result, (GDestroyNotify) g_variant_unref);
> 
> seems right to me. I almost wonder if this function should be gio ...

You mean g_clear_variant? Same could be said for g_clear_hash_table too, I guess. However, the thing that really irks me is having to write a one-line GDestroyNotify that does:
    g_list_free_full (list, g_object_unref);

(In reply to Ray Strode [halfline] from comment #10)
> Review of attachment 347638 [details] [review] [review]:
> ::: src/daemon/goadaemon.c
> @@ +1508,3 @@
> +
> +  task_queued = G_TASK (g_queue_pop_head (self->ensure_credentials_queue));
> +  g_assert (task == task_queued);
> 
> this is fine, but another way you could do it is..
> 
> g_assert (g_queue_peek_head (self->ensur...) == user_data);
> 
> task = G_TASK (g_queue_pop_head (self->ensur...));
> 
> Would get rid of the barely used extra GTask declaration then. But it's just
> a subjective style thing, really.  Also, I'm not sure which style I like
> better.  I'm just throwing it out there as an alternative.

As a matter of style, I try to avoid using user_data after the initial cast to the right type to keep the proliferation of void pointers in check. Plus, spelling out the "task" makes the assertion marginally more readable for me.

Halfline, thanks for the quick reviews and the extensive debugging!

Pushed to master, gnome-3-22 and gnome-3-20.

(In reply to Debarshi Ray from comment #11)
> (In reply to Ray Strode [halfline] from comment #9)
> > ::: src/goabackend/goakerberosprovider.c
> > @@ +1360,3 @@
> >   out:
> >    g_clear_pointer (&iter, (GDestroyNotify) g_variant_iter_free);
> > +  g_clear_pointer (&result, (GDestroyNotify) g_variant_unref);
> > 
> > seems right to me. I almost wonder if this function should be gio ...
> 
> You mean g_clear_variant?
no, i mean, i wonder if gio should have g_dbus_proxy_reload_properties_sync.  But let's just forget I brought it up unless we find another bug in the function :-)