GNOME Bugzilla – Bug 678584
Reload configuration when you send the daemon a HUP signal
Last modified: 2012-06-25 16:17:41 UTC
Created attachment 216981 [details] [review] patch adding this feature. This patch makes the GDM daemon reload the configuration when it receives a HUP signal. I find it useful for making GDM notice changes to the debug setting or to make Automatic/Timed login features start taking effect.
Created attachment 216982 [details] [review] Patch fixing issue Patch in better fornat
Review of attachment 216982 [details] [review]: sounds okayish, though maybe we should use a g_file_monitor and just automatically reread the file when it's closed ? ::: common/gdm-settings-direct.c @@ +236,3 @@ + if (schemas != NULL) + g_hash_table_destroy (schemas); + schemas = NULL; these days g_hash_table_unref is en vogue. Also, could use g_clear_pointer (&schemas, (GDestroyNotify) g_hash_table_destroy); for slightly smaller, prettier code. If you do stick with the if (...), then you need braces around it, and should pull the schemas = NULL into the braces. ::: common/gdm-settings.c @@ +263,3 @@ +{ + g_object_unref (settings_object); + settings_object = NULL; could use g_clear_object (&settings_object); here, though... Looking in the code I see: g_object_add_weak_pointer (settings_object, (gpointer *) &settings_object); So you don't need this function. You can just unref the object in main and gdm-settings.c will automatically nullify it's static pointer. ::: daemon/main.c @@ +480,3 @@ + if (! gdm_settings_direct_init (settings, GDMCONFDIR "/gdm.schemas", "/")) { + g_warning ("Unable to initialize settings"); + } so one concern is what if the admin adds a typo that makes the config fail to work? Maybe we should fall back to the already loaded settings in that case. How about instead of destroying the object, add a reload method to it?
Created attachment 217047 [details] [review] Updated patch I believe this patch addresses most of your concerns. Having some fallback to handle messed up configuration seems a nice-to-have feature. The only time gdm-binary should receive a HUP signal is if the sysadmin is trying to make it reload some configuration. If they do that and have an issue, they can just fix the problem and send another HUP to recover. If people want to make the code smarter, that's fine with me, but I think this provides the basic needed feature.
Review of attachment 217047 [details] [review]: ::: gdm-2.30.7/common/gdm-settings-direct.c-orig @@ +234,3 @@ + g_debug ("Settings Direct Init"); + g_hash_table_unref (schemas); need to check if schemas is NULL before calling unref on it. could just do g_clear_pointer (&schemas, (GDestroyNotify) g_hash_table_unref) and then you don't have to check for NULL or assign to NULL afterward.
Created attachment 217052 [details] [review] Updated patch How's this for an updated patch. I've tested this version and it also works, and I think addresses all of your concerns. GDM's configure.ac still only requires 2.29.3 while g_clear_pointer wasn't added until 2.34. If you want to make GDM depend on the newer GLIB and make changes like this, I'm all for it. But, anyway, my system doesn't even have a new enough version of glib to test it with.