*_timeout functions are used between update key and update greters calls to make sure that daemon has updated all the config values before greeter attempt config re-read. Some key values do not utilise *_timeout functions perfroming update greeters calls straight after updating a key hence causing a race condition. Im not entirely sure how many of those values are there (i can tell you GDM_KEY_LOGO is defenately on of them). Timeout functions use timeout intervals (i.e. wait intervals). As different keys/groups of keys have different timeout intervals im starting to think that their values were determined on a trial and error basis. Also those timeouts cause lags within interface responses (i.e.
there are pauses betwen selection and interface update). 
After a quick discussion with Brian we thought moving the update code into the deamon/slave block and getting rid of all the
timeout functions would remove the possibility of any race condition occurring, would remove a lot of existing cruft, clean up the code and would make the user experience a bit nicer.
This could be achieved by adding additional command:
UPDATE_REREAD_CONFIG which would re-read config in the daemon (just as UPDATE_CONFIG does) and then afterwards it would inform all the greeters that they need to re-read their config files (possibly by sending HUP signal)

Note that the daemon sends NOTIFY signals to the slave for certain configuration options.  This code can be found in daemon/gdmconfig.c.  Basically the daemon is doing the same thing, passing the updated key value to the slave for certain keys and sending the HUP signal to the slave to update.  Currently the daemon only sends NOTIFY for a few keys which is why this mechanism doesn't work for most options that are set via gdmsetup.  It's a bit gross that both the daemon and gdmsetup have separate mechanisms to try and update the GUI's - especially since the one in gdmsetup has the issues (race conditions, etc.) mentioned in the bug report above.

It would probably be better if we removed the logic in gdmsetup where it tries to manage updating the daemon config and sending the greeter's the HUP signals.  Instead we should enhance the daemon so it sends notify signals for all GUI related configuration options (basically all options in the [gui] and [greeter] sections).  This way, gdmsetup just tells the daemon to UPDATE_CONFIG, and the GUI's get notify signals and update.

The advantage of doing this is that if a user calls 'gdmflexiserver --command=UPDATE_CONFIG', then this will cause any running GUI's to update without needing to send the GUI a separate HUP signal by hand.  In other words, updating configuration options will work as expected not just with gdmsetup but also with the UPDATE_CONFIG command.

One annoyance with my suggested approach is that it causes the GUI's to update every time anything changes in gdmsetup.  It might be nicer if gdmsetup cached all changes and then when you exit gdmsetup it popped up a dialog like "Apply changes?".  If yes, then it could update the configuration at once and cause the GUI's to refresh just once.  This would avoid needless traffic telling the GUI's to update over and over again when the user is changing multiple options.  Also some keys are pretty nasty (e.g. Welcome message - see bug #335712) where the GUI's are updated every time you change a character in the input field.  If we are going to redesign this, it might be nice to address this issue as well.

>Note that the daemon sends NOTIFY signals to the slave for certain
>configuration options.  This code can be found in daemon/gdmconfig.c. 
>Basically the daemon is doing the same thing, passing the updated key value to
>the slave for certain keys and sending the HUP signal to the slave to update. 
>Currently the daemon only sends NOTIFY for a few keys which is why this
>mechanism doesn't work for most options that are set via gdmsetup.

True the number of these options is small and each one of them requires separate notofication key, e.g. for GDM_KEY_GREETER its corresponding notification key is called GDM_NOTIFY_GREETER and equal to "Greeter". notify_displays_* passes that as an argument to the slave which in turn processes each of the notification keys in gdm_slave_handle_notify. gdm_slave_handle_notify seems to be clever enough to distinguish whcih options need greeter restart and which should invoke config re-read. e.g.
.
.
.
} else if (sscanf (msg, GDM_NOTIFY_SYSTEM_MENU " %d", &val) == 1) {
gdm_set_value_bool (GDM_KEY_SYSTEM_MENU, val);
if (d->greetpid > 1)
kill (d->greetpid, SIGHUP);
.
.
.
do_restart_greeter = TRUE;
if (restart_greeter_now) {
; /* will get restarted later */
} else if (d->type == TYPE_STATIC) {
/* FIXME: can't handle flexi servers like this
 * without going all cranky */
if ( ! d->logged_in) {
gdm_slave_quick_exit (DISPLAY_REMANAGE);
} else {
remanage_asap = TRUE;
}
}
}

What puzzles me though is that for each config key there is a corresponding notification key.
If we were to make changes i think we should use the orignal keys rather than notification keys, as adding all of the config options would be a nightmare and flexibility would be non-existant in case of future modifcation (unless there was a particular reason why such distinction was introduced).
What we could do is create a default behaviour for gdm_slave_handle_notify which would be kill (d->greetpid, SIGHUP). All the options that need greater restart would be processed accordingly. Using standard keys as parameters would allow us to use one notify_displays_* call per each _gdm_set_value_* rather than multiple calls nested in if/else cascade.

>One annoyance with my suggested approach is that it causes the GUI's to update
>every time anything changes in gdmsetup.  It might be nicer if gdmsetup cached
>all changes and then when you exit gdmsetup it popped up a dialog like "Apply
>changes?". 

I think caching could be easily done. Atm CustomCommands use sort of caching, where all the values are stored in a temp hash table and once user clicked apply the hash is purged and values are saved. We could easily add handlers to the close action of the dialog that would perform the change (and probaby intercept kill signalls too). However there are few things we need to consider:
1. Atm all the existing values (against which the new ones are compared) are retreived from the daemon (GET_CONFIG) - they would have to point to the cache
2. using UPDATE_CONFIG wouldn't be feasible anymore. Each of the values parsed causes a config reread (or would if we ought to make modifications mentioned above). Doing multiple parameters at once might not allow update of all the parameters (i.e. if the first parameters causes greeter restart the remaining ones might not get picked up - another race condition). We could use command which would cause the daemon to re-read entire config (without sending any notification to the slaves) then send another command that would cause the greeter rereads (HUP signal)
3. Existing Apply Changes functionality for Include/Exclude users and custom commands would have to go (we would be working on the cache)
4. Im not sure if the changes application on close would suffice. Maybe having an apply button on the interface would be a good idea? (just in case users want to apply changes and keep browsing/changing other options). Or maybe im just too used to the way its working atm and my brain is having trouble adapting to the change?

As for bug #335712 the problem would be solved by using the cache (i think)

I think the reason the daemon uses notify keys is that there isn't really a good reason why the GUI's should be restarted when configuration changes.  Most config options could probably be handled by just telling the GUI there is a new value and letting the GUI update itself.  However, looking at the gdm_reread_config function we can see that this hasn't really been implemented.  Instead the GUI's tend to restart when they notice any key has changed.

One thing to notice, though, is that gdmlogin and gdmgreeter use many different keys so you probably don't want to restart the GUI's if a gdmlogin key changes but we are using gdmgreeter.  This probably wouldn't happen using gdmsetup, though, since you have to change the GUI before it lets you modify configuration values that affect it.

I'd say lets keep the NOTIFY mechanism if we plan to make the GUI's smarter and update themselves without restarting.  If we just want to admit defeat and agree that restarting the GUI's on configuration changes makes the most sense, then making the daemon more simply restart the GUI's on configuration change makes sense.

I agree that just restarting the GUI's with a HUP makes more sense if we fix gdmsetup so it caches all configuration options and updates them all at once when the user hits "Apply changes".

Regarding your points
1) Right, we would need to validate against a cache.  This might not be so hard 
   to implement since gui/gdmconfig.c already supports keeping things in a 
   cache.
2) It might make more sense to enhance UPDATE_CONFIG to take a new option
   that says "avoid restarting GUI's".  Then when we send the last UPDATE_CONFIG
   we call UPDATE_CONFIG and tell it to do the restart.
3) Correct.  We would need to get rid of current Apply buttons and replace it
   with an "Apply" button at the bottom of the screen (probably with a pop
   up in case the user quits without hitting Apply)
4) Yes, I think having an Apply button at the bottom would be a good idea with
   a pop-up if the user hits Close without hitting Apply first (assuming they
   made a change).

Yes, I think bug 335712 would be fixed by doing this work.  I think this work
would make gdmgreeter work better and remove a lot of ugly code from GDM that
doesn't really work well and is hard to maintain (as we can see since some
keys like LOGO don't work right).

>One thing to notice, though, is that gdmlogin and gdmgreeter use many different
>keys so you probably don't want to restart the GUI's if a gdmlogin key changes
>but we are using gdmgreeter.  This probably wouldn't happen using gdmsetup,
>though, since you have to change the GUI before it lets you modify
>configuration values that affect it.

We could tailor gdm_reread_config functions in both greeters such that they restart/update only on the values relevant to them.

>I'd say lets keep the NOTIFY mechanism if we plan to make the GUI's smarter and
>update themselves without restarting.  

I dind't mean to ditch the NOTIFY mechanism, only to modify it slightly. 
if (is_key (key, GDM_KEY_GREETER))
         notify_displays_string (GDM_NOTIFY_GREETER, *setting);

notify_displays_string is invoked with GDM_NOTIFY_GREETER rather than GDM_KEY_GREETER, where,
#define GDM_NOTIFY_GREETER "Greeter" /* <greeter binary> */

So it looks like for every config key there is a separate notification key which is equal to the config key name minus the section part.
What i was suggesting is to ditch the notion of notification keys (not the notification mechanism) and use config keys instead, so rather than
if (is_key (key, GDM_KEY_GREETER))
         notify_displays_string (GDM_NOTIFY_GREETER, *setting);

we would have 
if (is_key (key, GDM_KEY_GREETER))
         notify_displays_string (GDM_KEY_GREETER, *setting);

and GDM_KEY_GREETER would be truncated to "daemon/Greeter=" string. In this way adding a new option to the notifcation mechanism wouldn't involve creating separate NOTIFICATION key and creation of "if clauses" that process it. We could even have one statement in _gdm_config_set_* like:
notify_displays_string (key, *setting);
and let the slaves handle the appropriate updates.

Well, currently the gdm_reread_config functions should be tailored to only respond to keys that they care about.  So this logic just needs to stay the same.  

Does it work if you change the notify mechanism to pass the key value rather than the GDM_KEY_GREETER key?  I'm not sure of the impact of this change, so it might be helpful if you explain your analysis a bit more to ensure that we won't break anything by changing the way this works.

I do think it would be better if we modified this so that it is more generic and doesn't require a separate NOTIFY key for each key.  This would make the code much more clean.  However, I just want to make sure it doesn't break anything.  Are there any assumptions about when the GDM_NOTIFY_GREETER notiication key is used that needs to be preserved?

>Well, currently the gdm_reread_config functions should be tailored to only
>respond to keys that they care about.  So this logic just needs to stay the
>same.  

Indeed. I wasn't planning on making any changes there. 

>Does it work if you change the notify mechanism to pass the key value rather
>than the GDM_KEY_GREETER key?  I'm not sure of the impact of this change, so it
>might be helpful if you explain your analysis a bit more to ensure that we
>won't break anything by changing the way this works.
>I do think it would be better if we modified this so that it is more generic
>and doesn't require a separate NOTIFY key for each key.  This would make the
>code much more clean.  However, I just want to make sure it doesn't break
>anything.  Are there any assumptions about when the GDM_NOTIFY_GREETER
>notiication key is used that needs to be preserved?

I don't think anthing would break. GDM_KEY_* contains key_part and its default value (which can be quite long). I'm guessing the reason for using separate NOTIFY keys (which are much much shorter in length) was to reduce overheads as then notification could occur quite often when gdmsetup was running (basically every time the value changed). Although in saying that GDM_KEY_* could be truncated by removing prefixed section name and postfixed default value. I will run some tests to see if changing that affects gdm behaviour in any way. If that is the case then cleaning up the code would be a great idea.

I have noticed that there are two independent mechanisms for updating the config values:
1. Using UPDATE_CONFIG and notify_displays*
2. Using gdm_reread_config through direct HUP signal to the greeter
It seems the above two methods overlap (and even 1 uses 2 on for few config values) so we should clean up the code

But,
clen-up of UPDATE_CONFIG is not gonna be the best solution to gdmsetup notification mechanism as it deals with updating and sending notifications for single keys only. So what i propose is:

Lets add new command called NOTIFY_GREETERS which will re-read entire config file (and update what is necessary) in the daemon and then send notifications (HUP) to all the greeters - gdm_reread_config functions will take care of the rest. As we discussed earlier gdmsetup should implement caching - we can gather all the values that need to be changed and then save them to the config files in one sweep. Once that is done we could send NOTIFY_GREETERS command to synchronise internal data with the updated config files. 

When that is tested and working we could clean-up UPDATE_CONFIG mechanism.

What od you think

For the most part your analysis looks good.  Wouldn't it be easier, though, to 
make a new UPDATE_CONFIG option?  Something like "UPDATE_CONFIG all" to tell GDM to re-read the entire configuration file.  Enhancing the existing UPDATE_CONFIG command seems a bit nicer than creating a new command.

Since the GUI currently re-reads all configuration options already in gdm_reread_config, it might not really be necessary to tell the GUI which keys have changed.  Furthermore, for 95-100% of the keys, the GUI simply restarts when it notices any key it cares about has changed.

It only adds value to pass the specific key to the GUI if the GUI is going to be smart enough to manage updating the key changing without restarting.  At least, this is my thinking.  Let me know if your understanding is different.

So perhaps we should just fix the code so gdmsetup sends a single "UPDATE_CONFIG all" when the user Applies the changes in gdmsetup, then the daemon re-reads all configuration options, then tells the GUI to call gdm_reread_config and figure out if it needs to restart or not.

Or perhaps we could just move this code into the daemon.  Let the daemon figure out if the GUI's need to restart, and just restart them without talking between the daemon and GUI at all.  This would require making the daemon a bit smarter about which GUI is running.  Obviously gdmlogin and gdmgreeter care about different keys - so the daemon would need to be aware of which keys the two GUI's use (e.g. the ones in the existing gdm_reread_config functions for the two programs are a bit different) and restart only if a key that the GUI cares about has changed.

So I'm suggesting that it might make sense to just rip out all this code.  If, in the future, someone wants to implement smart logic so that GUI's can update their configuration without restarting then we can add this feature back in if someone is willing to really implement this properly.

In short, I think this mechanism of having the GUI's be notified about key changes was never really implemented correctly.  Obviously, the idea was that the GUI's should update themselves without needing to restart - but that was never really implemented in the GUI side.  So we have a lot of chatter between the GUI's and the daemon for no good reason.

I think ripping out this code would make GDM a lot more efficient in its communication - especially on machines with multiple displays (e.g. terminal servers with hundreds of displays).  It doesn't really make sense to cause so much chatter between the daemon and GUI's when the end result is just that the GUI's restart themselves when any key changes.

I think i managed to implement the basic structure for the new notifier. (It was slighly more complex as i initially thought ;))
Heres a quick breakdown of things that i will change:
In gdmconfig.c:
Added a global flag
static gint greeters_update_flag = GDM_NOTIFY_FLAG_DO_NOTHING;
this can take on the following values:
#define GDM_NOTIFY_FLAG_DO_NOTHING            0x0
#define GDM_NOTIFY_FLAG_NO_RESTART            0x1
#define GDM_NOTIFY_FLAG_RESTART_STATIC        0x2
#define GDM_NOTIFY_FLAG_RESTART_XMDCP         0x4
#define GDM_NOTIFY_FLAG_RESTART_STATIC_XMDCP  0x6

This flag is used only by the "bulk" update

gdm_update_config function:
1. Added an "all" UPDATE_CONFIG option:
This reads both config files, resets greeters_update_flag to GDM_NOTIFY_FLAG_DO_NOTHING, then invokes "modified" g_hash_table_foreach function
and gdm_update_xservers functions. Finally it notifies the displays through notify_displays_int (GDM_NOTIFY_UPDATE_ALL_GREETERS, greeters_update_flag) only and only if greeters_update_flag is different to GDM_NOTIFY_FLAG_DO_NOTHING.

_gdm_set_value_* functions:
I have changed handling of the updates i.e. (by the example of _gdm_set_value_int):
/* Handle update */
if (*setting != setting_copy) {
   if (doing_update == TRUE) {
	   if (is_key (key, GDM_KEY_RETRY_DELAY) ||
	       is_key (key, GDM_KEY_TIMED_LOGIN_DELAY)) {
		   gchar *stripped_key, *p;	      
		   stripped_key = g_strdup (key);	      
		   g_strstrip (stripped_key);
		   p = strchr (stripped_key, '=');
		   if (p != NULL)
			   *p = '\0';
		   
		   notify_displays_int (stripped_key, *setting);
		   
		   g_free (stripped_key);	   
	   }
	}
	else {
	   if (is_key (key, GDM_KEY_TIMED_LOGIN_DELAY))
		   greeters_update_flag |= GDM_NOTIFY_FLAG_RESTART_STATIC_XMDCP;
	   else greeters_update_flag |= GDM_NOTIFY_FLAG_NO_RESTART;
	}
   }
}

In the doing_update case the notification is performed by a "section/keyname" part rather than separate GDM_NOTIFY_* macro (All the GDM_NOTIFY_* were defined as a key name minus the section name). Now it is much easier to add an additional key that we want the slaves to be notified about without adding new macros and creating nested if/else structures.
In the other case we only interested in setting greeters_update_flag which will define future action of the slave(s). 

In slave.c:
gdm_slave_handle_notify:
gchar **splitstr = g_strsplit (msg, " ", 2);
if (splitstr[0] != NULL) {
	if (splitstr[1] != NULL) {
		if (is_key (splitstr[0], GDM_KEY_ALLOW_ROOT) ||
		    is_key (splitstr[0], GDM_KEY_ALLOW_REMOTE_ROOT) ||
		    is_key (splitstr[0], GDM_KEY_ALLOW_REMOTE_AUTOLOGIN)) {		
			if (sscanf (splitstr[1], "%d", &val) == 1)
				gdm_set_value_bool (splitstr[0], val);

This format retains the old functionality without need for separate KEY_NOTIFY_* macros. There is one issue i have noticed though: most of the keys use gdm_set_value_* calls. Is it the case that the code in gdm_slave_handle_usr2_message (GDM_SLAVE_NOTIFY_KEY section) is triggered ONLY by a notify_displays* calls through _gdm_set_value_*? If it is then we could ditch all the gdm_set_value_* calls in gdm_slave_handle_notify and further simplify its structure (there is no point in trying to set the same value twice)

There is a little anomaly i have noticed:
gdm_slave_handle_usr2_message only appends values-to-be-processed to the unhandled_notifiers list which then gets purged by check_notifies_now in various places (and by the looks of things check_notifies_now only handles GDM_SLAVE_NOTIFY_KEY requests). So i did the following:
1. Left the greeter on username prompt
2. Edited custom.conf and change value of Greeter field in [daemon] section (from gdmlogin to gdmgreeter or vice versa depending what you running atm)
3. Run the following command gdmflexiserver --command="UPDATE_CONFIG daemon/Greeter"

Result:
The greeter does not restart untill something is entered into the username field and enter is pressed. Brian can you confirm that this behaviour is the same in your case?

I think the cayse of this weird behaviour is the following line in the gdm_slave_wait_for_login function:
login = gdm_verify_user (d /* the display */,
			username /* username*/,
			d->name /* display name */,
			d->attached /* display attached? (bool) */);
it causes "wait" between subsequent calls to check_notifies_now and hence any update caught there will be held until the gdm_verify_user completes.
P.S. I have tested the latest svn head code (without my modifications) and it exhibits the same behaviour.

Would it be safe to add a call to check_notifies_now after the gdm_slave_handle_usr2_message processed GDM_SLAVE_NOTIFY_KEY request without risk of any blocking occuring (I'm still trying to get my head round the information in bug #336549)? It seems like this is a one-way communication and maybe it would be good to process the information as it comes along

Created attachment 84450 [details] [review]
Created against latest (Todays) SVN trunk

I have used this patch to test the anomaly mentioned above (Note: I have used gdm_info rather than gdm_debug so you wont have to enable the debug mode):
1. Started gdm (using default gdmlogin)
The following was present in /var/log/messages:
Mar 12 20:09:19 dragonorb gdm[21605]: check_notifies_now on display :0: unhandled notifies size 0

2. Added Greeter=/usr/libexec/gdmgreeter to the [deamon] section in custom.conf

3. Executed gdmflexiserver --command="UPDATE_CONFIG daemon/Greeter" and got the following in /var/log/messages:
Mar 12 20:09:25 dragonorb last message repeated 9 times
Mar 12 20:14:27 dragonorb gdm[21604]: (child 21605) gdm_slave_handle_usr2_message on display :0: read from slave fd value Greeter /usr/libexec/gdmgreeter
Where last message repeated 9 times was a reference to 1.
(It looks like my previous claims about check_notifies_now not being run were unfound but still the greeter did not restart)

4. Typed something in the Username filed and pressed enter - greeter restarted and got the following:
Mar 12 20:20:01 dragonorb gdm[21605]: check_notifies_now on display :0: unhandled notifies size 1
Mar 12 20:20:01 dragonorb gdm[21605]: gdm_slave_handle_notify on display :0: handling notification key Greeter with value /usr/libexec/gdmgreeter
Mar 12 20:20:01 dragonorb gdm[21630]: check_notifies_now on display :0: unhandled notifies size 0

5. Changed Greeter=/usr/libexec/gdmlogin in the [deamon] section in custom.conf

6. Executed gdmflexiserver --command="UPDATE_CONFIG daemon/Greeter" and got:
Mar 12 20:21:32 dragonorb gdm[21630]: check_notifies_now on display :0: unhandled notifies size 0
Mar 12 20:21:33 dragonorb last message repeated 7 times
Mar 12 20:22:54 dragonorb gdm[21604]: (child 21630) gdm_slave_handle_usr2_message on display :0: read from slave fd value Greeter /usr/libexec/gdmlogin

7. Typed something in the Username filed and pressed enter - greeter restarted:
Mar 12 20:24:37 dragonorb gdm[21630]: check_notifies_now on display :0: unhandled notifies size 1
Mar 12 20:24:37 dragonorb gdm[21630]: gdm_slave_handle_notify on display :0: handling notification key Greeter with value /usr/libexec/gdmlogin
Mar 12 20:24:37 dragonorb gdm[21862]: check_notifies_now on display :0: unhandled notifies size 0

Hmmm it looks like check_notifies_now is only called when the list is empty.

Actually i was right the first time. There are periods when check_notifies_now is not executed - these include username prompt and password prompt. 
Mar 12 20:21:33 dragonorb last message repeated 7 times
was referring to the 7 calls to check_notifies_now that happened before inacactivity period was engaged.
So although we can append new notifiers onto unhandled_notifiers list whenever we please (through USR2 signal) the list does not get purged during regular intervals. This is not particularly good from the point of view of the new config update notification mechanism. One of the solutions would be adding call to check_notifies_now () at the end of gdm_slave_usr2_handler function (ie.e we purge straight after we received the data). Does that sound plausible?

Sorry i meant put check_notifies_now () call inside gdm_slave_handle_usr2_message right after the data has been appended to the unhandled_notifiers list.

I'm a little confused.  GDM currently seems to restart the GUI if it is at the username prompt, so I'm guessing something about changing the code the way we are discussing breaks that?  If so, why did this used to work but not after the change?  I'd appreciate if you could explain this a bit more.

It's a little bad that GDM will restart the greeters in the middle of the user typing their username/password.  It might be nicer if GDM waited until GDM was idle for a while (maybe 30 seconds) before restarting if the user is typing.  I think all the GUI configuration options only affect the appearance of the GUI, so the user would likely be annoyed to have their login interrupted to see a new logo or something in the GUI.  If the user authenticates, there is no need to update the GUI since the GDM GUI will go away anyway and the user session will begin.

However, maybe this isn't a big deal.  People probably don't modify the configuration much while other users may be in the middle of the login process, so people aren't bothered by it enough to complain.  The current code works this way (as you mention), so I don't mind if it continues to work the same way.  I just think it might be nicer if it didn't restart in the middle of the user typing.  

I don't see the patch that implements this feature in this bug report, only your "test patch" in comment #10.  Could you share the patch that implements this feature for review?

Overall, from what you've said about it, it sounds like a good improvement to the way GDM works for the many reasons we've discussed so far in the bug report.

>I'm a little confused.  GDM currently seems to restart the GUI if it is at the
>username prompt, so I'm guessing something about changing the code the way we
>are discussing breaks that?  If so, why did this used to work but not after the
>change?  I'd appreciate if you could explain this a bit more.

Its not the case anymore - well not for me anyway. And this is without any of the changes i have made. I have even tested 2.16.5 release (bundled with FC6) and it still does not want to restart the greeters as soon as the daemon is notified of the change. This is how i understand it should behave (with the example of daemon/Greeter key entry which is one of the options that should restart the greeters):
1. custom.conf entry daemon/Greeter is modified
2. gdmflexiserver --command="UPDATE_CONFIG daemon/Greeter" is executed
3. gdm daemon then invokes gdm_update_config function which in turn invokes one of the gdm_set_value_* functions (in the case of the example it is gdm_set_value_string). Then notify_displays_string is invoked that notifies all the displays (through SIGUSR2)
4. each of the displays processes its SIGUSR2 handler (gdm_slave_usr2_handler) by appending received notification to the unhandled_notifiers list.
5. check_notifies_now is invoked at the regular intervals which in turn calls gdm_slave_handle_notify which processes the notifies and executes appropriate action

However in my case (i have tested it on two separate machines and two different versions of gdm - 2.16.5 and 2.17.X but the same) #5 does not happend at the regular intervals but is bound by a linear code execution path - hence the gaps between the calls to check_notifies_now.
Again im not entirely sure that this is working as it supposed to work. 
Brian, does the greeter restart as soon as you issue UPDATE_CONFIG command (and when its sitting idle waiting for username/password to be typed) or does it happened after you press enter on the username/password stages hence moving down the code execution path?

>It's a little bad that GDM will restart the greeters in the middle of the user
>typing their username/password.
>People probably don't modify the
>configuration much while other users may be in the middle of the login process,
>so people aren't bothered by it enough to complain.

Actually atm it waits until user confirms his/hers input by pressing enter or OK button. In terms on the new gdmsetup - daemon notification mechanism this is suboptimal though. After the changes are "applied" the notification process should restart greeter(s) as soon as possible rather than wait.

>I don't see the patch that implements this feature in this bug report, only
>your "test patch" in comment #10.  Could you share the patch that implements
>this feature for review?

Sorry for that i will submit the patch once i finished testing - i don't think its ready yet as i need to add the new "cache" functionality to gdmsetup. So far i have been stuck in the slave.c code trying to figure out why the notification behaves so oddly

Can i make an assumption that GDM_SLAVE_NOTIFY_COMMAND is only triggered by notify_displays_* through gdm_set_value_* functions? I have serched around the source code and i think the assumption is sound. The reason im asking is if that is the case we could further simplify gdm_slave_handle_notify by removing all the calls to gdm_set_value_* from it. Am I missing something?

Yes, slave_handle_notify is a nasty bit of code.  Why do you think the slave daemon doesn't need to call set upon receiving notification of the config change?  Remember the slave daemon is a different process than the main daemon, so calling set in the main daemon doesn't cause the slave daemon to update.

The main daemon starts a "slave" daemon for each display and the slave daemon starts the GUI.  So there are really 1 + 2 processes per display programs that need to be updated anytime a configuration value changes.

This function is making sure that the slave daemon itself knows about any changes to configuration values that it cares about.  Note that the slave daemon does access some configuration values, so it needs to know (along with the GUI) what values have changed.

So, I don't think we can remove the gdm_set_value calls here, but the current code is probably broken.  The slave daemon should likely just update itself upon any configuration value change.  I think the current code is trying to be smart and only update the slave via notify when something changes that it should care about.

However, this approach probably leads to bugs since it is easy to forget to update this function upon adding new configuration values that affect the slave.  Since the slave and main daemon share the same codebase, it is easy for a configuration value to be added to the main daemon but not added to the slave update function.  So, any such configuration values won't update in the slave daemon.

It would probably be better if the slave received notification on any change and updated all configuration values.  It would be a bit slower, but you would be sure that all processes have the same understanding of the configuration.

Does this make sense?

It might be good to add some debug logic to the code where you print the old and new values to syslog so you can verify that the slave daemon is getting updated properly.

Lucasz, how is this bug going?  I know that the churn in head is probably causing a headache for this patch - but hopefully not too bad.  I'd really like to get this fixed in SVN head, so I'm hoping you might be close to having something ready?

Also, I really want to get bug #164786 fixed, and I think fixing this is a step towards doing that.  Please see my comments in that bug.  If you want to help with fixing that, it would be great too.

Brian sorry for the delay. Im off on eastrer break will be back home on Monday. Then i need to make some adjustments as some of the "config" code has changed recently. Id say hopefuly it should be ready at the end of next week.

Any update?  It's now the "end of next week".  :)

Brian still workin on it :( will let you know as soon as its done. Sorry for the delay

Created attachment 87375 [details] [review]
built against yesterday's svn HEAD 

Sorry for the delay, next week all of the sudden became next few weeks, but finally here is the first part of the patch  :)
This patch adds functionality in the daemon/slave environment. Now the all/PARAMETERS option is available through gdm_daemon_config_update_key function (and consequently through gdmfexiserver --command "UPDATE_CONFIG ...") which in essence re-loads all the parameters (except the "forbidden" and the xserver ones). Depending on the parameters that were re-loaded appropriate notification is sent to each of the slaves informing them if they have to restart or just re-load their configuration(s). Also single key notification is performed by the actual config keys strings (i.e. group/key pairs) rather than separate GDM_NOTIFY_* keys (each of the GDM_NOTIFY_* was defined as a key name with prefixed group removed anyways). 

Code in gdm_slave_handle_notify (slave.c) has been cleaned up. Now for each of the notification varaibles we re-read entire config rather than just set its value as it was done previously. this presents slightly more overhead but it keeps all config files in sync. All the "notified" iterms are grouped according to the following action (i.e. restart static, restart xdmcp, restart both, send HUP).

Now im going to focus on gdmsetup (as a second part of this bug). Im thinking of re-writing most of it as the present notification mechanism wont be used anymore   and i think better conceptual grouping onf the functions/elements could be achieved (atm they are grouped mostly according to their widget type rather that to the tab/dialog they are in)

In the meantime it would be nice to give part1 of the patch run for its money ;)

William, since this patch affects the config code, could you also review this?  Adding you to the cc:list.

I have a few comments about this patch:

1) In daemon/slave.c it seems that there could be further cleanup.  I see this 
   block of code repeated:

+                                       do_restart_greeter = TRUE;
+                                       if (restart_greeter_now) {
+                                               ; /* will get restarted later */
+                                       } else if (d->type == TYPE_XDMCP) {
+                                               /* FIXME: can't handle flexi servers like this
+                                                * without going all cranky */
+                                               if ( ! d->logged_in) {
+                                                       gdm_slave_quick_exit (DISPLAY_REMANAGE);
+                                               } else {
+                                                       remanage_asap = TRUE;
+                                               }gdm
+                                       }

  The only difference is in the "else if" whether we test against TYPE_XDMCP or
  TYPE_STATIC, etc.  Perhaps this could be made a function that takes TYPE as an 
  argument?

2) You seem to create a notify_status global in daemon/gdm-daemon-config.c
   and you pass it in as 2nd argument to gdm_config_set_notify_status_func.
   Globals are ugly, we should avoid using them.

   This callback does a bunch of stuff to set this status, but unless I'm
   missing something, this isn't used for anything (aside from perhaps 
   printing out the debug message in notify_status_cb).  What is the 
   purpose of this?  I notice that these same FLAGS are tested in your
   changes to slave.c where you process GDM_NOTIFY_UPDATE_ALL_GREETERS, but
   I don't see GDM_NOTIFY_UPDATE_ALL_GREETERS getting sent from anywhere
   in this patch.  Is this because it is expected this would only get sent
   from gdmsetup?  If so, I'm a bit confused since gdmsetup doesn't currently
   sent NOTIFY directly to the daemon.  Instead it just sends UPDATE_CONFIG
   commands...so I'd expect "UPDATE_CONFIG all" or something to generate
   the GDM_NOTIFY_UPDATE_ALL_GREETERS or something. 

3) I'm a bit confused by the notify_status_cb which seems to only check a
   few keys, setting GDM_NOTIFY_FLAG_RESTART_STATIC|XDMCP, etc.  Is this logic
   used to restart the GUI or the slave daemon?

   - If the GUI, note that gdmlogin and gdmgreeter restart itself anyway when 
     it receives a HUP signal and then calls the reread function and notices any 
     key value it cares about actually changed.  Since this logic isn't 
     written to consider STATIC versus XDMCP etc. I imagine this doesn't really
     work to well.
   - If the slave daemon, I suspect that there are other keys that should cause
     the slave daemon to restart.  By this I mean that the slave itself does
     use several keys.

   If certain keys should only cause certain types of greeters to be restarted 
   (STATIC|XDMCP|etc) then we should probably review all the configuration keys 
   and determine which ones apply to which types of greeters (what about 
   DefaultWelcome and DefaultRemoteWelcome and BackgroundRemoteOnlyColor for 
   example).

   Note that in gdm-daemon-config-entries.h we setup an array for each key 
   entry.  Perhaps it would make more sense for each GdmConfigEntry to store 
   this value (GDM_NOTIFY_FLAG_RESTART_STATIC|XDMCP|etc) rather than looking it
   up in a case statement?

4) You add updatable_value_func_data to the structure, but don't actually use
   this.  You just pass in NULL here:

+       gdm_config_set_updatable_value_func (daemon_config, updatable_value_cb, NULL);

   Why not just remove the updatable_value_func_data from the structure and just 
   avoid setting it to NULL?

   Also why does the updatable_value_func accept so many arguments when it
   actually only needs "id"?

  +               res = (* config->updatable_value_func) (config, source, entry->group, entry->key, value, entry->id, config->updata
ble_value_func_data);

   Likewise the notify_status_cb only uses data and id (it only uses group
   and key for the debug message which isn't really needed since you could
   just print out they key id instead (or lookup the string via the id).
   So why does this function take 7 argument when it only needs a few?

5) Could you explain the need to call config->notify_func versus 
   config->notify_status_func?


-       if (config->notify_func) {
-               (* config->notify_func) (config, source, group, key, value, id, config->notify_func_data);
+       if (do_notify) {
+               if (config->notify_func) {
+                       (* config->notify_func) (config, source, group, key, value, id, config->notify_func_data);
+               }
+       }
+       else {
+               if (config->notify_status_func) {
+                       (* config->notify_status_func) (config, source, group, key, value, id, config->notify_status_func_data);
+               }
        }

6) You add a bunch of new functions like gdm_config_process_entries_update,
   process_all_update, process_all_no_notify, process_all_no_notify_update.
   Could you explain why so many cases are needed?  Some don't seem to be
   called from anywhere (it seems only gdm_config_process_all_no_notify_update
   is actually used).

7) I hate to be picky, but I am finding it hard to review the patch with 
   so much white-space cleanup.  For example in common/gdm-config.h it seems
   you changed the whitespace for all the functions.  Could you update the
   patch to not include white space cleanup changes mixed with this patch.
   This logic is complicated to understand, and trying to sort cleanup from
   actual code change is making this harder to review. 

>1) In daemon/slave.c it seems that there could be further cleanup.  I see this
>   block of code repeated:
[snip]

understood. Will change it

>2) You seem to create a notify_status global in daemon/gdm-daemon-config.c
>   and you pass it in as 2nd argument to gdm_config_set_notify_status_func.
[snip]

I do agree globals are ugly. However this corresponds to a daemon_config varaible (also global) in gdm-daemon-config.c so i thought i make it global as well. We could add it to the GdmConfig structure but this particular varaible/feature is not used that often.

Now let me explain the logic behind notify_status:
This flag is mostly used during bulk update. We cant really predict which config entries have changed and most importantly what action needs to be applied to the slave(s). So the callback function (notify_status_cb in daemon/gdm-daemon-config.c) gets called for every single valid config entry we process (internal_set_value in common/gdm-config.c) and status of the action is updated. So for some of the variables we might wish to re-load greeter(s) for some we need to restart them. This all gets recorded in the notify_status flag. So instead of using notification mechanism for every single config entry we process we use notify_status as cache which at the end is purged.

>I don't see GDM_NOTIFY_UPDATE_ALL_GREETERS getting sent from anywhere in this >patch.
its actually sent from gdm_daemon_config_update_key in daemon/gdm-daemon-config.c:
.
.
.
if (is_key (keystring, "all/PARAMETERS")) {
.
.
.
notify_displays_value (daemon_config, "all", "PARAMETERS", notify_value);
.
.
.

>Instead it just sends UPDATE_CONFIG commands...so I'd expect "UPDATE_CONFIG >all" or something to generate the GDM_NOTIFY_UPDATE_ALL_GREETERS or something. 

Correct. In the past gdmsetup would use UPDATE_CONFIG and HUP signal to all slaves for every config value changed (For values that were using existing notification mechanism HUP signal was not sent from gdmsetup if im correct). Now we would cache all the changes then save them all in one go and then send UPDATE_CONFIG all/PARAMETERS (also same thing can be done through gdmflexiserver --command).

>3) I'm a bit confused by the notify_status_cb which seems to only check a
[snip]
GDM_NOTIFY_FLAG_RESTART_STATIC|XDMCP are used to restart slave daemon (and consequently the GUI as well). GDM_NOTIFY_FLAG_NO_RESTART is used to restart only GUI. The default behavior is restart GUI only specific keys require restart of the slave daemon.

>If certain keys should only cause certain types of greeters to be restarted >(STATIC|XDMCP|etc) then we should probably review all the configuration keys >and determine which ones apply to which types of greeters

I agree there are probably some keys that should appear there - i have only used the ones that were there before. Some of the keys will be featured in GUI configurations and will cause relevant updates. The keys that we should add are the keys that are used in the slaves and do not appear in the greeters.

>Perhaps it would make more sense for each GdmConfigEntry to store this value >(GDM_NOTIFY_FLAG_RESTART_STATIC|XDMCP|etc) rather than looking it up in a case >statement?

Sounds like a good idea.

>4) You add updatable_value_func_data to the structure, but don't actually use >this. 

I have kep them simmilar to all other callback functions for GdmSetup i.e. validate_func, notify_func, .... with the syntax defined in the common/gdm-common.h. 
But you right only handful of the parameters is used.

>5) Could you explain the need to call config->notify_func versus >config->notify_status_func?

notify_func actually sends a notification signal to all the slaves (in the form of !KEYSTRING value, where ! is defined as GDM_SLAVE_NOTIFY_KEY). 
notify_status_func only updates the status flag with the action that should be applied to the slave - it does not send any notification signals

>6) You add a bunch of new functions like gdm_config_process_entries_update
[snip]
The functions you mentioned were ment to be an "higher abstraction layer" functions. all the process_all* functions could be wrapped round into one  gdm_config_process_all (GdmConfig *config, GError **error, gboolean do_notify, gboolean  doing_update) function. Then all the calls to gdm_config_process_all would have to be adjusted. As the bulk update code is not used very often providing conceptual functions and leaving the rest of the code alone seemed like a good idea. The functions that are not used anywhere were provided for completeness sake and can be removed.

> 7) I hate to be picky, but I am finding it hard to review the patch with
[snip]
You absolutely right didnt realise it caused such havoc untill i had a look myself - will get it fixed

Created attachment 87496 [details] [review]
build against two days old svn HEAD

Issues 1) and 7) fixed

Thanks, the patch is now easier to review:

1) I think the code looks much better with #1 fixed.

2) I'm not sure "lookup_notify_key" is a good name for the function in 
   gdm_daemon_cofig.c now that it only returns the keystring.  Perhaps this 
   function should be renamed to something like "get_notify_key"?

3) In regards to item #4 above, I don't really see the value in making these
   two new callbacks follow the same "conventions", especially having the
   unused updatable_value_func_data variable.  I think this variable should 
   be removed from the structure and we shouldn't set it if we are only going to 
   ever set it to NULL.  I think changing the function params to only include 
   the ones used would be nice, but I wouldn't reject the patch if you didn't 
   change this.

4) In regards to #6 above, it seems that the various functions like
   gdm_config_process_all_update, gdm_config_process_all_no_notify, etc.
   just call process_entries with two different arguments (TRUE versus FALSE).
   I'd prefer to see a single function and pass in extra booleans so that
   the caller can pass the right flags in rather than having separate
   functions that do the same thing but just make the call with different flags.

5) There seems to be some white-space usage seems to be inconsistant in 
   places (like in the notify_status_cb function).

Aside from these issues (which are all pretty minor), I'm happy with the
patch.  So I will accept the patch if you address the above.

Sorry for the delay.  I've been swamped.  I've taken a quick look at the patch but I don't like the gdm-config changes at all.  The way to think about gdm-config is to pretend that it isn't part of gdm and that it should really not have gdm specific functionality.

Also policy like whether a key is updatable or not should probably be handled by the caller.

Making the structure public is not a good idea.

The slave notification thing is pretty gross mostly because of the design of GdmDisplay and the IPC between master and slave.  Once we have a better GdmDisplay and D-Bus this should be a lot simpler to fix.

Created attachment 87739 [details] [review]
build against five days old svn HEAD

Brian,

2) Fixed
3) I would like to keep it as it is
4) Fixed all the functions are now represented with one (as it was originally) with two boolean parameters

Jon,

>Making the structure public is not a good idea.
I have removed public acces to the structure (with the addition of a getter function)

>The way to think about
>gdm-config is to pretend that it isn't part of gdm and that it should really
>not have gdm specific functionality.

I do agree, but gdm-config already contains "gdm specific" functionality like notification callback function/data. They are defined in gdm-daemon-config but called upon in internal_set_value. One could remove the do_notify and doing_update parameters passed through gdm_config_process_all, gdm_config_process_entry and gdm_config_process_entries and use the data variable of the respective callback functions (notify_status_func, updatable_value_func) to achieve the same behaviour

>Also policy like whether a key is updatable or not should probably be handled
>by the caller.
This could bloat the existing code and duplicate already existing functionality. The mechanism for iterating through every config entry of loaded config file, checking its validity and comparing it against existing config entry has already been implemented very nicely

Lukasz:

Thanks for your patience here.  This area of the code is in the ugly guts of
GDM, so its important to make sure we get the logic right.

I think we are getting closer here.  This patch is starting to look good to me.  I do agree with Jon that we should minimize the impact to the gdm-common code.  Here is what I think:

1) Do we really need the common logic to worry about which keys are updatable?
   Currently the gdm_daemon_config_update_key function in gdm-daemon-config.c
   rejects keys that are not "updatable".  Why can't we just keep using the
   old logic rather than moving this into the common code?  All we want to do 
   here is make UPDATE_KEY not accept keys that are (currently) not supported.  
   Your change just moves this test into the common code, but as Jon points out, 
   this is daemon specific.  Also this gets rid of the updatable_value_func_data 
   which I don't like since it is always NULL.

2) I don't think the getter function gdm_config_get_notify_status_func_data is
   really needed.  This function is only called from daemon/gdm-config.c which
   is where the notify_status variable is defined.  So can't we just test 
   notify_status directly here?  We shouldn't have to pull the variable out of
   the internal common structure to access it since it is just a pointer to 
   notify_status.

3) I think that it is appropriate for the common code to keep track of the
   notify_status since we want to enhance GDM to be able to handle updates in
   bulk, rather than just one at a time, so I think this is okay.

I think this addresses Jon's concerns (along with the work you have already done to keep the GdmConfig structure private).  

Does this make sense?

1) Rejection of the "non-updatable" keys in gdm_daemon_config_update_key works fine with single keys and known groups of keys (like xdmcp/PARAMETERS). With bulk update we dont really know which keys we need to update. In order to move the "non-updatable" logic out of gdm-config.c we would have to replicate some of the  functionality already present in there i.e. 
iterate through every key check if its "updatable", and then set the value with gdm_config_set_value with do_notify flag set to FALSE (which isn't there atm).

2) The addition of gdm_config_get_notify_status_func_data was necessary since the _GdmConfig structure was made "private" again (moved to gdm-config.c). Any attempts of accessing the internals of the structure directly will result in compile error: dereferencing pointer to incomplete type. We could make additional utility functions that set the status_data internally without

3) I think so too. Also i think being able to specify which keys are "updatable" and which are not is probably less gdm specific than the notification status.

Sorry pressed Save changes too quickly.
2) The addition of gdm_config_get_notify_status_func_data was necessary since
the _GdmConfig structure was made "private" again (moved to gdm-config.c). Any
attempts of accessing the internals of the structure directly will result in
compile error: dereferencing pointer to incomplete type. We could add
additional utility function (such as gdm_config_set_notify_status_func_data) that sets the status_data internally without need to extract the pointer to perform operations on it.

Lukasz:

Again, thanks for being patient.  I think I understand the approach being used here better now, and am more comfortable with this patch.  

Perhaps it would be good to summarize what we are trying to do here, so William can understand better.  There are a number of problems with the way that GDM currently handles notify.  These issues include:

- gdmsetup is too chatty and calls UPDATE_CONFIG for every change you make.
  This causes GDM to restart the slave daemon and the GUI's over and over
  again, which is a waste.  It would be better if gdmsetup just had an Apply
  button that you pressed after making all changes.  Then the main daemon could
  restart everything just once.

- The current notify logic is ugly and difficult to maintain, with separate
  NOTIFY keys for each configuration value that causes a restart.  This patch
  makes the NOTIFY logic easier to manage since we use the actual key value 
  as the notify key.

- As this bug report highlights, there are some race conditions with the way
  gdmsetup works.  gdmsetup currently has ugly timeout functions that cause 
  the UPDATE_CONFIG to be sent after a 500ms delay.  Going with a single 
  Apply button approach allows us to get rid of this ugly code.

- The current logic in the GUI is rather dumb and restarts the GUI when any
  config that affects the GUI changes.  It would be better if this logic were
  made a bit smarter.

- A longer term goal would be to avoid running gdmsetup as root.  Once we 
  update with a single Apply button, we can fix gdmsetup so it does the apply
  with privilege rather than running the entire GUI as root.  It will be easier
  to do this when gdmsetup is fixed to do a single Apply rather than updating
  as you go.

So, I agree with Lukasz, we need logic to support the ability to tell the
daemon to update all configuration changes at once.  The current approach is
to send the daemon to update "all/PARAMETERS" which causes a re-read of all
configuration data.

So, I think we do need the two mechanisms Lukasz has added to make this work. 
The ability to keep track of which keys cause slave daemon to restart (currently notify_status_cb) and the ability to keep track of which keys should not be
updated (currently updatable_value_cb).

So, William, does this help you understand what the code is doing?  You have
expressed some issues with this code change, so I'd appreciate if you could
make some suggestions if there is a better way to achieve this.

Having said all that, I still disagree with your point #2 above.  Currently the 
patch does this:

+		GdmNotifyStatus *status = (GdmNotifyStatus*)gdm_config_get_notify_status_func_data (daemon_config);
+		g_return_val_if_fail (status != NULL, FALSE);
+		*status = GDM_NOTIFY_FLAG_DO_NOTHING;

However, the gdm_config_get_notify_status function is only going to return
a pointer to the locally defined notify_status variable because this is the
variable that we set as the data when we call gdm_config_set_notify_status_func.
Couldn't the code just set notify_status = GDM_NOTIFY_FLAG_DO_NOTHING.  Since
the gdm-config logic just contains a pointer to notify_status as the callback
data, this should cause the gdm-config logic to see the change as well.

Created attachment 87864 [details] [review]
built against the seven days old svn HEAD 

>Having said all that, I still disagree with your point #2 above
[snip]
You are right Brian. It completely slipped my mind that we using notify_status as global variable. Fixed in this patch

Okay, Lukasz.  I am now happy with this patch.  I'll leave this open for a bit longer to see if William wants to comment further.  If he has any further suggestions about how to improve the logic here, we can address those as we go.
If I don't hear from him in the next day or so, I'll assume he is okay with this.

Coding XDMCP or STATIC display specific stuff in GdmConfig is just not acceptable.    This stuff has nothing to do with a config mechanism.

Also, in my opinion the NOTIFY stuff is just broken beyond repair and we need to replace it rather than try to patch it up.

Okay, I think this has an easy fix.  William's main problem seems to be that the 
GdmNotifyStatus enumeration is defined in the common code (where it isn't used, by the way).  It is only used in the daemon code in gdm-daemon-config.c, so I think moving this enumeration to gdm-daemon-config.h is what we want to do here.

I agree that the NOTIFY stuff is broken, but Lucasz is working to make it better.  
If we rewrite it later to use d-Bus or whatever, then we can fix it at that time again.

William, are you okay with this?

Created attachment 87994 [details] [review]
build against nine days old svn HEAD

I have moved GdmNotifyStatus enumeration into gdm-socket-protocol.h as i think thats where it more less conceptually belongs. It is used in socket communcation and gdm-dameon-config.c and slave.c are the only places that it appears.

Brian, William are you ok with that?

>I agree that the NOTIFY stuff is broken ...
Indeed it is pretty nasty :). 
I would like to finish sorting out gdmsetup first (this would allow us to close few gdmsetup rleated bugs) before we start thinking/changing daeamon/slave to use d-bus.

Lukasz.  I'm happy with your patch.  I think this can go upstream.  But I'm going on vacation for 2 weeks.  Is it okay if I don't put it upstream until I get back?  

Also, looking forward to seeing phase 2 of the patch.

Thats fine Brian. It should give me enough time to finish phase 2 of the patch.
Have a good holiday :) 

Any progress, Lukasz?  I'm back from vacation now.  :)

Sorry Brian some delay on my side. Been swamped by work in the last couple of weeks :(
How do you think we should handle the save/notification of changes:
1. Shall we save to the config file every time we make changes (old-style) and do  notify all on exit
2. (Preferable) We cache all the changes and save them when user clicks on save changes button or on exit.

If we choose #2 where shall we put "Apply changes" button? I think it should go in the main interface (where Help and Close buttons are). But then shall we substitute Close with "Apply changes" - which then will give option to save & continue, save & close, or just close? Or do we add "Apply changes" as additional button?

I'd prefer option #2.  It would be nice if we wrote a separate program that we could call like this:

gdmconfig-update key value

Then gdmsetup could call this program with sudo or gksu (popping up a enter password dialog) in order to allow the GUI to run as the normal user and the part of the program that requires additional security (writing the configuration file) is handled by a separate program).

I think there should be 3 buttons.  Help, Apply, and Quit.  If you pick Quit without saving changes, a dialog box should pop up and ask "Are you sure?".

Oki doki :)
One more question though. The recent announcement of new GObject branch (great job William btw) made me think if we should adopt GObject-like structure for gdmsetup. All of the notify-timeout code for widgets can be removed now, and it probably would be nice to clean up the remaining code. 
Brian, William what are your thoughts.

This bug report is probably already overly long, with 43 comments already.  I'd recommend pinging William via email and seeing if you could work with him to get gdmsetup working as we would like in the new GObject branch.  Could you work with him on this?

Maybe it might make sense, though, to wait until the GObject branch is a bit more usable than it is at the moment.

Oki doki Brian i will contact William shortly. I think you right it would probably mean going to a lot of trouble to get things integrated with existing structure. I will make some small modifucations to gdmsetup to suit our current purpose and leave the major restructuring to the GObject release

Created attachment 90788 [details] [review]
Beta version 1

Finally :) 
This is a beta version of the patch.
There are still few bugs in the code (im working on atm) and printf debug statements but i think structure should stay more less the same.
This patch includes all the changes discussed earlier plus the follwing gdmsetup changes:
1. Now gdmsetup uses local cache to store the daemon config values (except [servers] and [server-*] sections). When the value is requested for the first time its loaded through gdm_config_get_* functions - all subsequent requests are handled from the cache. 
There is a problem however: setup_user_combobox_list uses call to gdm_users_init (from gdmuser.c) which was using gdm_config_get_* internally - hence we are using local cache those values did not correspond to the currently set values (before hitting apply button). I have provided little wrapper function (gdm_users_init_full) but then noticed that gdm_config_get_* calls are used in other functions used there. Maybe re-write fo that functionality in gdmsetup context is at hand
2. User changes are stored in a cache until Apply button is clicked then they are written to the config file and the daemon is notified.
3. All "widget" timeout functions have been removed.
4. [servers] and [server-*] sections are loaded from the config file. Theoretically (i have not tested that yet) gdmsetup will allow modification of user defined [server-*] sections. One still wont be able to add/remove those sections through the interface though (most of the code is already in place but it needs to be finalised)

Unfortunately gdmsetup still runs as root - it is on the TODO list but not in this patch. 

There are probably other things i should mention but its quite late - it will come to me in the morning

Created attachment 90840 [details] [review]
Beta version 2

Fixed few more bugs.
Still more testing to do, few more bugs to fix

Created attachment 90986 [details] [review]
Final (i hope) but built against quite old svn

Brian,
I will not make any changes to this patch untill you give me some feedback/comments.

I have also changed the way gdmlogin handles config updates. Previously some of the values didnt need greeter restart (like logo, welcome message, background colour, etc) but updating those fields wasn't producing expected results (and i think it goes for the previous versions of gdm as well). I still have not got to the root of the problem, but visually it demonstrated itself in gdmlogin spuriously changing background colour (even if the background colour change was not requested) and not accepting any more HUP signals (re-read config). Now gdmlogin restarts after every update (like gdmgreeter does and did in the past).

Since we've already passed the freeze dates for 2.19/2.20, I think this will need to wait until 2.21.  Note that when the D-Bus rewrite goes in, this will need some additional work since the communication between daemon/slave/greeter is different in the rewrite.  If the D-Bus rewrite goes into 2.21, then this may need to be changed at that time.  If the D-Bus rewrite goes into 2.23, then I'll accept this patch in at that time.

Note, I haven't yet reviewed this patch yet in detail, but will do so early in the 2.21 cycle when we will have a better feel if the D-Bus rewrite will go into 2.21 or later.

Of course. If it goes into the next release it might be necessary to modify this patch so it fits nicely with up2date svn head (it has been a while since this patch was compiled)

*** Bug 449735 has been marked as a duplicate of this bug. ***

It might be good to break up that monster patch into 2 parts.  One patch that is things we could apply today to 2.19 to fix real problems users are having, and a second patch that implements the "improve the race condition and how the greeters are updated with new configuration changes". 

Lukasz, would you be up for providing a new patch that fixes the more serious and obvious problems in the big patch, so we can get some issues fixed in the 2.20 timeframe.  Remember that since we are past freeze date we can only fix issues that don't affect strings and are bugs (rather than enhancing the GUI with new functionality).

You don't need to provide the "second patch" (the one that fixes this bug of how the greeters are updated with new configuration changes) at the moment, just the "first patch" to fix the serious problems we want fixed in 2.19 in the near future.  We can worry about porting the "second" patch later when we need to for 2.21.

Thanks for taking the time to report this bug.
However, you are using a version that is too old and not supported anymore. GNOME developers are no longer working on that version, so unfortunately there will not be any bug fixes for the version that you use.

By upgrading to a newer version of GNOME you could receive bug fixes and new functionality. You may need to upgrade your Linux distribution to obtain a newer version of GNOME.
Please feel free to reopen this bug if the problem still occurs with a newer version of GNOME.