Created attachment 164082 [details] [review]
Update copy and paste version of gdmuser

Sync with GDM master.  Fixes many serious performance and
sanity errors.  Requires ConsoleKit from git master.

Created attachment 164083 [details] [review]
Update to new gdmuser

Actually it fails gracefully if you don't have the new CK.  Just prints out:
** (ck-history:20964): WARNING **: Unknown option --since=2010-04-20T16:03:55.319450Z

Review of attachment 164082 [details] [review]:

::: src/gdmuser/gdm-user-manager.c
@@ +61,1 @@
 #define CK_SESSION_INTERFACE "org.freedesktop.ConsoleKit.Session"

CK_PATH and CK_INTERFACE aren't used

@@ +76,2 @@
 #define DEFAULT_GLOBAL_FACE_DIR DATADIR "/faces"
+#define DEFAULT_USER_ICON       "avatar-default"

DEFAULT_GLOBAL_FACE_DIR and DEFAULT_USER_ICON aren't used

@@ +394,3 @@
+
+        error = NULL;
+        res = dbus_g_proxy_call (manager->priv->seat_proxy,

boo, more blocking calls

(i guess this will appear in the wrong order in the review... i complain more about blocking dbus calls later on)

@@ +579,3 @@
+        if (a == NULL || b == NULL) {
+                return -1;
+        }

This seems unnecessary: you never call g_slist_find_custom() with a NULL item anyway so you don't have to worry about a NULL == NULL match (and if you did call it with NULL, it would still only end up matching if the caller had erroneously added a NULL element to the exclude list, and then they deserve to lose anyway).

@@ +689,3 @@
+        g_object_unref (user);
+
+        user = g_hash_table_lookup (manager->priv->users_by_name, pwent->pw_name);

what is the point of that shuffle?

@@ +737,3 @@
+                          G_CALLBACK (on_user_sessions_changed), manager);
+        g_signal_connect (user, "changed",
+                          G_CALLBACK (on_user_changed), manager);

you should move the signal connections into add_user() rather than doing them independently in both create_user() and add_new_user_for_object_path().

@@ +764,3 @@
+        g_object_ref (user);
+        remove_user (manager, user);
+        g_object_unref (user);

you should move the ref/unref of user into remove_user(); otherwise the call to remove_user() in reload_passwd_job_done() could run into the same problem

@@ +1431,3 @@
+        sigemptyset (&child_set);
+        sigaddset (&child_set, SIGCHLD);
+        sigaddset (&child_set, SIGPIPE);

libdbus sets SIGPIPE to SIG_IGN, so you don't have to worry about that

@@ +1464,3 @@
+        block_sigchld ();
+
+        status = kill (pid, signal);

I'm pretty sure that it's not guaranteed that the SIGCHLD will arrive before you call unblock_sigchld(). You need to use sigsuspend(). But really, why do you need to block signals here at all?

If it's just to keep ck_history_watch() from processing a partial response, then there are better ways to deal with that, like checking if manager->priv->ck_history_watchdog_id has been cleared.

@@ +1689,3 @@
+
+                        /* ...And explicitly excluded users */
+                        if (exclude_users != NULL) {

you can just call user_in_exclude_list() here (which gets rid of the GDM_USERNAME check above as well)

@@ +1746,3 @@
         fclose (fp);
 
+        g_hash_table_destroy (new_users_by_name);

you lost the code that calls g_object_thaw_notify() on each of them

@@ +1767,3 @@
+        }
+
+        g_slist_foreach (data->added_users, (GFunc) g_object_ref, NULL);

*unref*, not ref (likewise for removed_users below)

@@ +1855,3 @@
+        g_debug ("GdmUserManager: scheduling a passwd file update");
+
+        g_io_scheduler_push_job ((GIOSchedulerJobFunc) do_reload_passwd_job,

If you're running this in another thread, then passwd_data needs to make copies of all the data you're passing it, or else, eg, the exclude_usernames list could be modified in the main thread while it was being read from the ioscheduler thread.

@@ +1999,3 @@
+        }
+
+        manager->priv->reload_passwd_id = g_timeout_add_seconds (5, (GSourceFunc)reload_passwd_idle, manager);

this function is pretty mysterious. why is it necessary to wait until 5 seconds after the last notification?

@@ +2290,3 @@
+                                                              g_str_equal,
+                                                              g_free,
+                                                              (GDestroyNotify) g_object_run_dispose);

That looks like a workaround for a bug somewhere else. Possibly an earlier workaround for the need-to-ref/unref-around-emitting-USER_REMOVED thing.

At any rate, since you ref users when adding them to the table in add_user(), and don't unref them in remove_user(), this means you're leaking all the users. (They get disposed, but not finalized.)

It should be g_object_unref. If that causes problems somewhere else then there's a bug somewhere else.

@@ +2295,3 @@
+                                                                     g_str_equal,
+                                                                     NULL,
+                                                                     (GDestroyNotify) g_object_unref);

FWIW g_object_unref already has the form of a GDestroyNotify, so you don't need a cast

@@ +2313,1 @@
+        manager->priv->cancellable = g_cancellable_new ();

You pass this to g_io_scheduler_push_job() in one place, but you never call g_cancellable_cancel() on it, or give a copy of it to anyone else who might... so it seems like it doesn't do anything and could just be removed.

@@ +2330,2 @@
         g_return_if_fail (manager->priv != NULL);
 

My dbus-glib-fu is weak, but you may be leaking priv->connection, priv->seat_proxy, and priv->accounts_proxy. Or maybe not.

::: src/gdmuser/gdm-user-manager.h
@@ +56,1 @@
                                                         GdmUser        *user);

fix indentation?

::: src/gdmuser/gdm-user.c
@@ +43,3 @@
 #define MAX_ICON_SIZE     128
 #define MAX_FILE_SIZE     65536
 #define MINIMAL_UID       100

MAX_ICON_SIZE and MINIMAL_UID are unused

@@ +192,3 @@
         g_free (user->user_name);
         g_free (user->real_name);
+        g_free (user->icon_file);

and user->object_path

@@ +235,3 @@
+                if (g_utf8_validate (pwent->pw_gecos, -1, NULL)) {
+                        valid_utf8_name = pwent->pw_gecos;
+                        first_comma = g_utf8_strchr (valid_utf8_name, -1, ',');

you can use plain strchr there. g_utf8_strchr is less efficient and is only actually needed when you're searching for a non-ASCII character. (though i guess g_utf8_strchr could be optimized...)

@@ +237,3 @@
+                        first_comma = g_utf8_strchr (valid_utf8_name, -1, ',');
+                } else {
+                        g_warning ("User %s has invalid UTF-8 in GECOS field. "

shouldn't you expect /etc/passwd to be in the locale encoding?

oh, hm, i see that it used to do that, and now it doesn't. Why? Is GDM no longer supporting non-UTF8 locales?

@@ +265,1 @@
              strcmp (real_name, user->real_name) != 0)) {

there are several places in this method where you do "if ((A && !B) || (B && !A) || (A && B && strcmp (A, B) != 0)", all of which can be simplified to just "if (g_strcmp0 (A, B) != 0)"

@@ +734,3 @@
+        if (user->icon_file) {
+                res = check_user_file (user->icon_file,
+                                       MAX_FILE_SIZE);

It's a bit confusing that you call check_user_file() from here, rather than from each place where you set user->icon_file, because that means that gdm_user_get_icon_file() may return a filename that gdm_user_render_icon() won't actually use.

(It's also weird-ish that check_user_file() takes a max_file_size arg, but then you just pass it a constant. It could just use the constant itself...)

@@ +773,3 @@
 }
+
+G_CONST_RETURN gchar *

G_CONST_RETURN is only for headers. Just say "const" in the .c file regardless of what you do in the .h

@@ +800,3 @@
+                g_debug ("User %s is not logged in, so has no primary session",
+                         gdm_user_get_user_name (user));
+                goto out;

seems like a silly use of goto here

@@ +809,3 @@
+
+                /* FIXME: better way to choose? */
+                if (ssid != NULL) {

actually, this whole function is silly. There's no way for a NULL value to end up in the sessions list. So if user->sessions is NULL, it should return NULL, and otherwise it should return user->sessions->data.

@@ +881,3 @@
+
+        error = NULL;
+        if (!dbus_g_proxy_call (proxy,

blah. synchronous call.

For the changed_handler case, you could just do this asynchronously and wait to emit CHANGED until after getting the values back, and it would all work fine.

The new_from_object_path case is a little trickier, but could all be fixed. (GAsyncInitable?)

Given that part of the goal of this patch set was to eliminate sync calls at shell startup...

::: src/gdmuser/gdm-user.h
@@ +41,3 @@
 
+GdmUser              *gdm_user_new_from_object_path (const gchar *path);
+const char           *gdm_user_get_object_path      (GdmUser *user);

you use G_CONST_RETURN rather than const on all of the other methods.

(although really, you should just change the other methods. G_CONST_RETURN is silly.)

Review of attachment 164083 [details] [review]:

looks ok

Cool.  Thanks for taking the time to review.  As you may gather the patch is an amalgamation of many commits from various people.  I'll respong in detail in the morning.

Review of attachment 164082 [details] [review]:

Will update.

::: src/gdmuser/gdm-user-manager.c
@@ +689,3 @@
+        g_object_unref (user);
+
+        user = g_hash_table_lookup (manager->priv->users_by_name, pwent->pw_name);

A little odd perhaps but it allows us to cleanly drop the ref and return a valid (null or not) pointer to the user without knowing what add_user did.  I've replaced this with a comment saying it always adds a ref and dropped the extra lookup.

@@ +1689,3 @@
+
+                        /* ...And explicitly excluded users */
+                        if (exclude_users != NULL) {

This is run in a thread.  If we do that, I guess have to guard against list being modified in the main thread.

@@ +1746,3 @@
         fclose (fp);
 
+        g_hash_table_destroy (new_users_by_name);

Ah, yeah I guess maybe we don't need the freeze since the objects don't have properties anymore.

@@ +1999,3 @@
+        }
+
+        manager->priv->reload_passwd_id = g_timeout_add_seconds (5, (GSourceFunc)reload_passwd_idle, manager);

To throttle reprocessing the entire passwd file.  Its expensive and there is no need to have it more frequent than that.  I've added a #define for this value.

@@ +2290,3 @@
+                                                              g_str_equal,
+                                                              g_free,
+                                                              (GDestroyNotify) g_object_run_dispose);

Indeed.  Wow, turns out this is from:
commit 8af653aad142a46b75e7ba078bc813796f8e3f5c
Author: James M. Cape <jcape@src.gnome.org>
Date:   Fri Apr 1 18:34:44 2005 +0000

    Initial revision

@@ +2313,1 @@
+        manager->priv->cancellable = g_cancellable_new ();

Yeah I had plans to use it be didn't.  Will drop it.

@@ +2330,2 @@
         g_return_if_fail (manager->priv != NULL);
 

Pretty sure that bus_get returns a global unref'd connection.  Seat proxy is unref'd below.  Account proxy is indeed leaked in a few cases.

::: src/gdmuser/gdm-user.c
@@ +237,3 @@
+                        first_comma = g_utf8_strchr (valid_utf8_name, -1, ',');
+                } else {
+                        g_warning ("User %s has invalid UTF-8 in GECOS field. "

Very good question.  I'll try to find out.  It's from:
commit 8b8521dd83637d4e83d67adddefba4f30c99715c
Author: Takao Fujiwara <tfujiwar@redhat.com>
Date:   Thu Oct 22 15:20:49 2009 +0900

    Fix real names not to crash greeter. Bug #594270
    
    Reviewed by Ray Strode

@@ +809,3 @@
+
+                /* FIXME: better way to choose? */
+                if (ssid != NULL) {

Yeah, the FIXME never got anything better than that.

I've filed bug 622639 to cover the "blocking calls" issues

*** This bug has been marked as a duplicate of bug 631888 ***