GNOME Bugzilla – Bug 620908
gdm user switch applet goes nuts while changing password file
Last modified: 2010-06-09 00:56:36 UTC
If I run a script like the following then the gdm-user-switch-applet consumes 100% cpu for the duration. #!/bin/sh for i in {1..10000} do name="boozer$i" useradd -M --no-user-group --no-log-init ${name} done That should be avoided.
Created attachment 163002 [details] [review] Ensure users-loaded signal is emitted
Created attachment 163003 [details] [review] Throttle reloading passwd file to at most every 5 seconds
Created attachment 163004 [details] [review] Convert users-loaded to an is-loaded property
Created attachment 163005 [details] [review] Revert "Update duplicate real names to be unique" This reverts commit dfc66ca1f169c8b9f1c504f762b3d01605917a98. This is a very expensive change and one that I think can be done in a better way. It doesn't make sense to me to have automatically generated and changing display names. From https://bugzilla.gnome.org/show_bug.cgi?id=549349
Created attachment 163006 [details] [review] Request the CK session list once instead of once per user
Review of attachment 163002 [details] [review]: Overall, this patch looks good. You also snuck in a "rename misnamed functions" change in there, which wouuld ideally go in a different commit, but no big deal. You do need a better commit message. In the description of the commit message maybe say something like "The users-loaded signal is used by the greeter to know when to mark the login dialog as 'ready'. This signal normally gets emitted after get a list of frequently logged in users from ConsoleKit. In the event the ConsoleKit daemon isn't available, however, the signal was never getting emitted. This commit causes the signal to get emitted from those cases as well" ?
Review of attachment 163003 [details] [review]: Looks good.
Review of attachment 163004 [details] [review]: Overall, this looks good. The commit message is too sparse though. ::: gui/simple-greeter/gdm-user-manager.c @@ +778,3 @@ + if (manager->priv->is_loaded) { + g_signal_emit (manager, signals[USER_ADDED], 0, user); + } This is a semantic change makes the commit message for this commit not exactly right. You're not only changing from a signal to a property, but you're now not emitting user-added signals for each user that makes the initial cut. That change should be either a separate commit, or the commit message for this commit should explicitly state this. You don't want people looking through history to think this change was unintentional.
Review of attachment 163005 [details] [review]: I feel like this commit needs strong justification. Right now if there are two users named "John Smith" and neither has set up a face image yet, they only way they'll be able to know which name to pick is from the tooltip. The "is very expensive" argument is really compelling, though. I think if we revert this our story needs be: 1) Adminstrators need to be leveraging the gecos field in a "gdm recommended" setup to prevent these kinds of ambiguities from happening in the first place 2) Ultimately, this is the kind of thing that accounts-dialog should be figuring out for us, not gdm That story should be in the commit message for code spelunkers.
Review of attachment 163006 [details] [review]: Looks really good, modulo a stale comment. ::: gui/simple-greeter/gdm-user-manager.c @@ -635,3 @@ - g_debug ("GdmUserManager: not adding session on other seat: %s", ssid); - goto out; - } I guess one important peice of information here is that the greeter is tied to a specific seat. It only cares about the things tha happen on that seat. This code is getting dropped, not moved (like say the x11_display check below) but it's not needed since add_session_for_user is only ever called in response to changes in the session make up of the specific seat. @@ +820,1 @@ /* only add the user if we added a session */ This comment is now confusing and wrong.
Thanks for the review. Committed to master with those changes. We should probably also add to 2.30.