Created attachment 276243 [details] [review]
goaidentity: don't leak default credentials cache

Review of attachment 276243 [details] [review]:

I think we are missing one hunk.

::: src/goaidentity/goakerberosidentitymanager.c
@@ -792,6 +792,1 @@
-      krb5_ccache default_cache;
-
-      error_code = krb5_cc_default (self->priv->kerberos_context, &default_cache);
... 3 more ...
+      error_code = krb5_cc_default (self->priv->kerberos_context, credentials_cache);

This is used in sign_in_identity. If error_code is 0 then it creates a new GoaKerberosIdentity. The krb5_ccache passed to goa_kerberos_identity_new is this one, and it makes a copy of it instead of stealing ownership. That means we should do krb5_cc_close irrespective whether the identity could be created or not.

Created attachment 276246 [details] [review]
goaidentity: one more ccache leak

goa_kerberos_identity_new copies the passed in credential cache,
so we should clean up the original on sign-in.

Spotted by rishi.

Review of attachment 276246 [details] [review]:

Looks good. You could squash it into the previous patch because it is part of the same thing.

::: src/goaidentity/goakerberosidentitymanager.c
@@ +854,3 @@
         }
+
+        krb5_cc_close (self->priv->kerberos_context, credentials_cache);

I am beginning to wonder if it would be more readable to get rid of the else corresponding to "if (error_code != 0)" and doing the krb5_cc_close right after the goa_kerberos_identity_new. But that is just a cosmetic thing that can be done separately later.

Review of attachment 276243 [details] [review]:

This is complete in combination with the other patch.

squashed and refactored like you suggested.