Created attachment 254263 [details] [review]
goaidentity: fall back to polling when monitoring is unavailable

We don't support getting change notification for all kerberos credential
cache types.  Rather than fail, in these cases, fall back to polling.

This futureproofs the code and gives more robustness in the case of
problems.

above is untested, but the idea is to mop up the general case, and then also add a non-polling variant for kernel keyring when it gains an api we can use.

(for clarity, i'm saying the kernel keyring patch will come later when there's an API to use)

Review of attachment 254263 [details] [review]:

::: src/goaidentity/goakerberosidentitymanager.c
@@ +1398,3 @@
     cache_path++;
 
   file = g_file_new_for_path (cache_path);

We're still creating a file for the cache, which will probably work since we just end up unreffing it, but it's still a bit ugly.

@@ +1425,3 @@
     {
+      goa_warning ("GoaKerberosIdentityManager: Could not monitor credentials for %s, reverting to polling: %s",
+                   cache_path, error->message);

"monitoring_error" right?

@@ +1426,3 @@
+      goa_warning ("GoaKerberosIdentityManager: Could not monitor credentials for %s, reverting to polling: %s",
+                   cache_path, error->message);
+      g_error_free (monitoring_error);

I always use g_clear_error so that if later code is added it won't be trying to overwrite a set but freed error.

@@ +1439,3 @@
 
+  if (require_polling)
+    self->priv->polling_timeout_id = g_timeout_add_seconds (5, (GSourceFunc) on_polling_timeout, self);

A #define at top instead of 5 is possibly nicer.

Created attachment 254269 [details] [review]
goaidentity: fall back to polling when monitoring is unavailable

We don't support getting change notification for all kerberos credential
cache types.  Rather than fail, in these cases, fall back to polling.

This futureproofs the code and gives more robustness in the case of
problems.

thanks, all good comments.  attachment 254269 [details] [review] tries to address them, but it still needs to get testing.

Review of attachment 254269 [details] [review]:

::: src/goaidentity/goakerberosidentitymanager.c
@@ +1430,3 @@
+      goa_warning ("GoaKerberosIdentityManager: Could not monitor credentials for %s, reverting to polling: %s",
+                   cache_path, monitoring_error->message);
+      g_clear_error (monitoring_error);

^ missing amperstand

Review of attachment 254269 [details] [review]:

::: src/goaidentity/goakerberosidentitymanager.c
@@ +1346,3 @@
   GFileMonitor *monitor;
   krb5_error_code error_code;
   GError *monitoring_error;

I think monitor and monitoring_error should be initialized to NULL. See below.

@@ +1429,3 @@
     {
+      goa_warning ("GoaKerberosIdentityManager: Could not monitor credentials for %s, reverting to polling: %s",
+                   cache_path, monitoring_error->message);

If we are here due to can_monitor being set to FALSE because the cache_type was neither "FILE" nor "DIR", then both monitor and monitoring_error are pointing to something undefined.

I think they should be initialized to NULL, and the goa_warning / g_clear_error should be enclosed in a "if (monitoring_error != NULL)"

@@ +1456,3 @@
+
+      g_object_unref (self->priv->credentials_cache_monitor);
+      self->priv->credentials_cache_monitor = NULL;

Could use  g_clear_object instead.

Created attachment 257004 [details] [review]
goaidentity: fall back to polling when monitoring is unavailable

We don't support getting change notification for all kerberos credential
cache types.  Rather than fail, in these cases, fall back to polling.

This futureproofs the code and gives more robustness in the case of
problems.

(still untested on my end, though I did run "make" this time)

one thing rishi and I noticed when talking on IRC is that there's still some code that assumes the credential cache resides on the filesystem.  We don't need that code anymore so we can just drop it.

Created attachment 257005 [details] [review]
goaidentity: drop dead code

We used to have to get the "raw credentials" from the kerberos
credential cache for realmd to use.  We no longer need that
so drop that code.

Created attachment 257006 [details] [review]
goaidentity: drop dead code

We used to have to get the "raw credentials" from the kerberos
credential cache for realmd to use.  We no longer need that
so drop that code.

So I've tested these patches locally now and they seem to work.

I did run into two problems when testing, but they're unrelated.  see bug 709953 and bug 709953 for details.

Review of attachment 257006 [details] [review]:

Sure.

Review of attachment 257004 [details] [review]:

Mostly looks good and works as expected.

::: src/goaidentity/goakerberosidentitymanager.c
@@ +1431,3 @@
+                   cache_path,
+                   cache_type,
+                   monitoring_error != NULL? monitoring_error->message : "");

You might want to use a "if (monitoring_error != NULL)" to only print a warning if there was an error. In those cases where "monitoring_error == NULL" we knowingly chose to poll because the cache_type was neither FILE nor DIR and we have already printed a warning when that happened.