Blob Blame Raw
From b1ec3114a49c9e5331a4f7c106a58867499ac1ce Mon Sep 17 00:00:00 2001
From: Ray Strode <rstrode@redhat.com>
Date: Wed, 4 Oct 2017 11:28:50 -0400
Subject: [PATCH 13/13] lib: only track users after act_user_manager_list_users

At the moment, as soon as someone calls
act_user_manager_get_default() we end up firing of an asynchronous
call to get a list of all users. This is problematic since not all
programs using accountsservice want a list of users.

This commit changes the code to only get a list of users when the
caller invokes act_user_manager_list_users.  This does mean some calls
that were async before are synchronous now, but user proxies were
always obtained synchronously, and they're by far the slowest part
of listing users, so I don't expect this introduce any noticeable
blocking.

Longer term, to fix the sync i/o bits, I think we should
probably ditch libaccountsservice and just make accountsservice use
ObjectManager interfaces over d-bus.
---
 src/libaccountsservice/act-user-manager.c | 145 +++++++++++++++---------------
 1 file changed, 75 insertions(+), 70 deletions(-)

diff --git a/src/libaccountsservice/act-user-manager.c b/src/libaccountsservice/act-user-manager.c
index 11049e0..d0b38e2 100644
--- a/src/libaccountsservice/act-user-manager.c
+++ b/src/libaccountsservice/act-user-manager.c
@@ -172,92 +172,93 @@ typedef struct
         };
         char                       *object_path;
         char                       *description;
 } ActUserManagerFetchUserRequest;
 
 struct ActUserManagerPrivate
 {
         GHashTable            *normal_users_by_name;
         GHashTable            *system_users_by_name;
         GHashTable            *users_by_object_path;
         GHashTable            *sessions;
         GDBusConnection       *connection;
         AccountsAccounts      *accounts_proxy;
         ConsoleKitManager     *ck_manager_proxy;
 
         ActUserManagerSeat     seat;
 
         GSList                *new_sessions;
         GSList                *new_users;
         GSList                *new_users_inhibiting_load;
         GSList                *fetch_user_requests;
 
         GSList                *exclude_usernames;
         GSList                *include_usernames;
 
         guint                  load_id;
 
         gboolean               is_loaded;
         gboolean               has_multiple_users;
         gboolean               getting_sessions;
-        gboolean               listing_cached_users;
         gboolean               list_cached_users_done;
 };
 
 enum {
         PROP_0,
         PROP_INCLUDE_USERNAMES_LIST,
         PROP_EXCLUDE_USERNAMES_LIST,
         PROP_IS_LOADED,
         PROP_HAS_MULTIPLE_USERS
 };
 
 enum {
         USER_ADDED,
         USER_REMOVED,
         USER_IS_LOGGED_IN_CHANGED,
         USER_CHANGED,
         LAST_SIGNAL
 };
 
 static guint signals [LAST_SIGNAL] = { 0, };
 
 static void     act_user_manager_class_init (ActUserManagerClass *klass);
 static void     act_user_manager_init       (ActUserManager      *user_manager);
 static void     act_user_manager_finalize   (GObject             *object);
 
 static gboolean ensure_accounts_proxy       (ActUserManager *manager);
 static gboolean load_seat_incrementally     (ActUserManager *manager);
 static void     unload_seat                 (ActUserManager *manager);
 static void     load_users                  (ActUserManager *manager);
+static void     load_user                   (ActUserManager *manager,
+                                             const char     *username);
 static void     act_user_manager_queue_load (ActUserManager *manager);
-static void     queue_load_seat_and_users   (ActUserManager *manager);
+static void     queue_load_seat             (ActUserManager *manager);
 
 static void     load_new_session_incrementally (ActUserManagerNewSession *new_session);
 static void     set_is_loaded (ActUserManager *manager, gboolean is_loaded);
 
 static void     on_new_user_loaded (ActUser        *user,
                                     GParamSpec     *pspec,
                                     ActUserManager *manager);
 static void     give_up (ActUserManager                 *manager,
                          ActUserManagerFetchUserRequest *request);
 static void     fetch_user_incrementally       (ActUserManagerFetchUserRequest *request);
 
 static void     maybe_set_is_loaded            (ActUserManager *manager);
 static void     update_user                    (ActUserManager *manager,
                                                 ActUser        *user);
 static gpointer user_manager_object = NULL;
 
 G_DEFINE_TYPE (ActUserManager, act_user_manager, G_TYPE_OBJECT)
 
 static const GDBusErrorEntry error_entries[] = {
         { ACT_USER_MANAGER_ERROR_FAILED,              "org.freedesktop.Accounts.Error.Failed" },
         { ACT_USER_MANAGER_ERROR_USER_EXISTS,         "org.freedesktop.Accounts.Error.UserExists" },
         { ACT_USER_MANAGER_ERROR_USER_DOES_NOT_EXIST, "org.freedesktop.Accounts.Error.UserDoesNotExist" },
         { ACT_USER_MANAGER_ERROR_PERMISSION_DENIED,   "org.freedesktop.Accounts.Error.PermissionDenied" },
         { ACT_USER_MANAGER_ERROR_NOT_SUPPORTED,       "org.freedesktop.Accounts.Error.NotSupported" }
 };
 
 GQuark
 act_user_manager_error_quark (void)
 {
         static volatile gsize ret = 0;
@@ -711,91 +712,87 @@ on_get_seat_id_finished (GObject        *object,
                          GAsyncResult   *result,
                          gpointer        data)
 {
         ConsoleKitSession *proxy = CONSOLE_KIT_SESSION (object);
         ActUserManager    *manager = data;
         GError            *error = NULL;
         char              *seat_id;
 
         if (!console_kit_session_call_get_seat_id_finish (proxy, &seat_id, result, &error)) {
                 if (error != NULL) {
                         g_debug ("Failed to identify the seat of the "
                                  "current session: %s",
                                  error->message);
                         g_error_free (error);
                 } else {
                         g_debug ("Failed to identify the seat of the "
                                  "current session");
                 }
 
                 g_debug ("ActUserManager: GetSeatId call failed, so unloading seat");
                 unload_seat (manager);
 
                 goto out;
         }
 
         g_debug ("ActUserManager: Found current seat: %s", seat_id);
 
         manager->priv->seat.id = seat_id;
         manager->priv->seat.state++;
 
-        load_seat_incrementally (manager);
-
  out:
         g_debug ("ActUserManager: unrefing manager owned by GetSeatId request");
         g_object_unref (manager);
 }
 
 #ifdef WITH_SYSTEMD
 static void
 _get_systemd_seat_id (ActUserManager *manager)
 {
         int   res;
         char *seat_id;
 
         res = sd_session_get_seat (NULL, &seat_id);
 
         if (res == -ENOENT) {
                 seat_id = NULL;
         } else if (res < 0) {
                 g_warning ("Could not get current seat: %s",
                            strerror (-res));
                 unload_seat (manager);
                 return;
         }
 
         manager->priv->seat.id = g_strdup (seat_id);
         free (seat_id);
 
         manager->priv->seat.state++;
-
-        queue_load_seat_incrementally (manager);
 }
 #endif
 
 static void
 get_seat_id_for_current_session (ActUserManager *manager)
 {
 #ifdef WITH_SYSTEMD
         if (LOGIND_RUNNING()) {
                 _get_systemd_seat_id (manager);
                 return;
         }
 #endif
         console_kit_session_call_get_seat_id (manager->priv->seat.session_proxy,
                                               NULL,
                                               on_get_seat_id_finished,
                                               g_object_ref (manager));
 }
 
 static gint
 match_name_cmpfunc (gconstpointer a,
                     gconstpointer b)
 {
         return g_strcmp0 ((char *) a,
                           (char *) b);
 }
 
 static gboolean
 username_in_exclude_list (ActUserManager *manager,
                           const char     *username)
 {
@@ -1520,106 +1517,64 @@ load_user_paths (ActUserManager       *manager,
         /* We now have a batch of unloaded users that we know about. Once that initial
          * batch is loaded up, we can mark the manager as loaded.
          *
          * (see on_new_user_loaded)
          */
         if (g_strv_length ((char **) user_paths) > 0) {
                 int i;
 
                 g_debug ("ActUserManager: ListCachedUsers finished, will set loaded property after list is fully loaded");
                 for (i = 0; user_paths[i] != NULL; i++) {
                         ActUser *user;
 
                         user = add_new_user_for_object_path (user_paths[i], manager);
                         if (!manager->priv->is_loaded) {
                                 manager->priv->new_users_inhibiting_load = g_slist_prepend (manager->priv->new_users_inhibiting_load, user);
                         }
                 }
         } else {
                 g_debug ("ActUserManager: ListCachedUsers finished with empty list, maybe setting loaded property now");
                 maybe_set_is_loaded (manager);
         }
 }
 
 static void
 load_included_usernames (ActUserManager *manager)
 {
         GSList *l;
 
         /* Add users who are specifically included */
         for (l = manager->priv->include_usernames; l != NULL; l = l->next) {
-                ActUser *user;
-
                 g_debug ("ActUserManager: Adding included user %s", (char *)l->data);
-                /*
-                 * The call to act_user_manager_get_user will add the user if it is
-                 * valid and not already in the hash.
-                 */
-                user = act_user_manager_get_user (manager, l->data);
-                if (user == NULL) {
-                        g_debug ("ActUserManager: unable to lookup user '%s'", (char *)l->data);
-                }
-        }
-}
-
-static void
-on_list_cached_users_finished (GObject      *object,
-                               GAsyncResult *result,
-                               gpointer      data)
-{
-        AccountsAccounts *proxy = ACCOUNTS_ACCOUNTS (object);
-        ActUserManager   *manager = data;
-        gchar           **user_paths;
-        GError           *error = NULL;
-
-        manager->priv->listing_cached_users = FALSE;
-        manager->priv->list_cached_users_done = TRUE;
-
-        if (!accounts_accounts_call_list_cached_users_finish (proxy, &user_paths, result, &error)) {
-                g_debug ("ActUserManager: ListCachedUsers failed: %s", error->message);
-                g_error_free (error);
-
-                g_object_unref (manager->priv->accounts_proxy);
-                manager->priv->accounts_proxy = NULL;
 
-                g_debug ("ActUserManager: unrefing manager owned by failed ListCachedUsers call");
-                g_object_unref (manager);
-                return;
+                load_user (manager, l->data);
         }
-
-        load_user_paths (manager, (const char * const *) user_paths);
-        g_strfreev (user_paths);
-
-        load_included_usernames (manager);
-
-        g_debug ("ActUserManager: unrefing manager owned by finished ListCachedUsers call");
-        g_object_unref (manager);
 }
 
 static void
 on_get_x11_display_finished (GObject      *object,
                              GAsyncResult *result,
                              gpointer      data)
 {
         ConsoleKitSession *proxy = CONSOLE_KIT_SESSION (object);
         ActUserManagerNewSession *new_session = data;
         GError            *error = NULL;
         char              *x11_display;
 
         new_session->pending_calls--;
 
         if (new_session->cancellable == NULL || g_cancellable_is_cancelled (new_session->cancellable)) {
                 unload_new_session (new_session);
                 return;
         }
 
         if (!console_kit_session_call_get_x11_display_finish (proxy, &x11_display, result, &error)) {
                 if (error != NULL) {
                         g_debug ("Failed to get the x11 display of session '%s': %s",
                                  new_session->id, error->message);
                         g_error_free (error);
                 } else {
                         g_debug ("Failed to get the x11 display of session '%s'",
                                  new_session->id);
                 }
                 unload_new_session (new_session);
                 return;
@@ -2361,60 +2316,99 @@ fetch_user_with_id_from_accounts_service (ActUserManager *manager,
  * from @manager. Trying to use this object before its
  * #ActUser:is-loaded property is %TRUE will result in undefined
  * behavior.
  *
  * Returns: (transfer none): #ActUser object
  **/
 ActUser *
 act_user_manager_get_user (ActUserManager *manager,
                            const char     *username)
 {
         ActUser *user;
 
         g_return_val_if_fail (ACT_IS_USER_MANAGER (manager), NULL);
         g_return_val_if_fail (username != NULL && username[0] != '\0', NULL);
 
         user = lookup_user_by_name (manager, username);
 
         /* if we don't have it loaded try to load it now */
         if (user == NULL) {
                 g_debug ("ActUserManager: trying to track new user with username %s", username);
                 user = create_new_user (manager);
 
                 if (manager->priv->accounts_proxy != NULL) {
                         fetch_user_with_username_from_accounts_service (manager, user, username);
                 }
         }
 
         return user;
 }
 
+static void
+load_user (ActUserManager *manager,
+           const char     *username)
+{
+        ActUser *user;
+        GError *error = NULL;
+        char *object_path = NULL;
+        gboolean user_found;
+
+        g_return_if_fail (ACT_IS_USER_MANAGER (manager));
+        g_return_if_fail (username != NULL && username[0] != '\0');
+
+        user = lookup_user_by_name (manager, username);
+
+        if (user == NULL) {
+                g_debug ("ActUserManager: trying to track new user with username %s", username);
+                user = create_new_user (manager);
+        }
+
+        user_found = accounts_accounts_call_find_user_by_name_sync (manager->priv->accounts_proxy,
+                                                                    username,
+                                                                    &object_path,
+                                                                    NULL,
+                                                                    &error);
+
+        if (!user_found) {
+                if (error != NULL) {
+                        g_debug ("ActUserManager: Failed to find user '%s': %s",
+                                 username, error->message);
+                        g_clear_error (&error);
+                } else {
+                        g_debug ("ActUserManager: Failed to find user '%s'",
+                                 username);
+                }
+        }
+
+        _act_user_update_from_object_path (user, object_path);
+}
+
 /**
  * act_user_manager_get_user_by_id:
  * @manager: the manager to query.
  * @id: the uid of the user to get.
  *
  * Retrieves a pointer to the #ActUser object for the user with the
  * given uid from @manager. Trying to use this object before its
  * #ActUser:is-loaded property is %TRUE will result in undefined
  * behavior.
  *
  * Returns: (transfer none): #ActUser object
  */
 ActUser *
 act_user_manager_get_user_by_id (ActUserManager *manager,
                                  uid_t           id)
 {
         ActUser *user;
         gchar  *object_path;
 
         g_return_val_if_fail (ACT_IS_USER_MANAGER (manager), NULL);
 
         object_path = g_strdup_printf ("/org/freedesktop/Accounts/User%lu", (gulong) id);
         user = g_hash_table_lookup (manager->priv->users_by_object_path, object_path);
         g_free (object_path);
 
         if (user != NULL) {
                 return g_object_ref (user);
         } else {
                 g_debug ("ActUserManager: trying to track new user with uid %lu", (gulong) id);
                 user = create_new_user (manager);
@@ -2425,93 +2419,95 @@ act_user_manager_get_user_by_id (ActUserManager *manager,
         }
 
         return user;
 }
 
 static void
 listify_hash_values_hfunc (gpointer key,
                            gpointer value,
                            gpointer user_data)
 {
         GSList **list = user_data;
 
         *list = g_slist_prepend (*list, value);
 }
 
 /**
  * act_user_manager_list_users:
  * @manager: a #ActUserManager
  *
  * Get a list of system user accounts
  *
  * Returns: (element-type ActUser) (transfer container): List of #ActUser objects
  */
 GSList *
 act_user_manager_list_users (ActUserManager *manager)
 {
         GSList *retval;
 
         g_return_val_if_fail (ACT_IS_USER_MANAGER (manager), NULL);
 
+        if (!manager->priv->list_cached_users_done) {
+                load_users (manager);
+
+                if (manager->priv->seat.state == ACT_USER_MANAGER_SEAT_STATE_GET_SEAT_PROXY)
+                        queue_load_seat_incrementally (manager);
+        }
+
         retval = NULL;
         g_hash_table_foreach (manager->priv->normal_users_by_name, listify_hash_values_hfunc, &retval);
 
         return g_slist_sort (retval, (GCompareFunc) act_user_collate);
 }
 
 static void
 maybe_set_is_loaded (ActUserManager *manager)
 {
         if (manager->priv->is_loaded) {
                 g_debug ("ActUserManager: already loaded, so not setting loaded property");
                 return;
         }
 
         if (manager->priv->getting_sessions) {
                 g_debug ("ActUserManager: GetSessions call pending, so not setting loaded property");
                 return;
         }
 
-        if (manager->priv->listing_cached_users) {
-                g_debug ("ActUserManager: Listing cached users, so not setting loaded property");
-                return;
-        }
-
         if (manager->priv->new_users_inhibiting_load != NULL) {
                 g_debug ("ActUserManager: Loading new users, so not setting loaded property");
                 return;
         }
 
-        /* Don't set is_loaded yet unless the seat is already loaded
+        /* Don't set is_loaded yet unless the seat is already loaded enough
          * or failed to load.
          */
-        if (manager->priv->seat.state == ACT_USER_MANAGER_SEAT_STATE_LOADED) {
+        if (manager->priv->seat.state >= ACT_USER_MANAGER_SEAT_STATE_GET_ID) {
                 g_debug ("ActUserManager: Seat loaded, so now setting loaded property");
         } else if (manager->priv->seat.state == ACT_USER_MANAGER_SEAT_STATE_UNLOADED) {
                 g_debug ("ActUserManager: Seat wouldn't load, so giving up on it and setting loaded property");
         } else {
                 g_debug ("ActUserManager: Seat still actively loading, so not setting loaded property");
                 return;
         }
 
         set_is_loaded (manager, TRUE);
 }
 
 
 static GSList *
 slist_deep_copy (const GSList *list)
 {
         GSList *retval;
         GSList *l;
 
         if (list == NULL)
                 return NULL;
 
         retval = g_slist_copy ((GSList *) list);
         for (l = retval; l != NULL; l = l->next) {
                 l->data = g_strdup (l->data);
         }
 
         return retval;
 }
 
 static void
@@ -2555,125 +2551,134 @@ static void
 load_console_kit_sessions (ActUserManager *manager)
 {
         if (manager->priv->seat.seat_proxy == NULL) {
                 g_debug ("ActUserManager: no seat proxy; can't load sessions");
                 return;
         }
 
         manager->priv->getting_sessions = TRUE;
         console_kit_seat_call_get_sessions (manager->priv->seat.seat_proxy,
                                             NULL,
                                             on_get_sessions_finished,
                                             g_object_ref (manager));
 }
 
 static void
 load_sessions (ActUserManager *manager)
 {
 #ifdef WITH_SYSTEMD
         if (LOGIND_RUNNING()) {
                 reload_systemd_sessions (manager);
                 maybe_set_is_loaded (manager);
                 return;
         }
 #endif
         load_console_kit_sessions (manager);
 }
 
 static void
 load_users (ActUserManager *manager)
 {
-        g_assert (manager->priv->accounts_proxy != NULL);
-        g_debug ("ActUserManager: calling 'ListCachedUsers'");
+        GError *error = NULL;
+        char **user_paths = NULL;
+        gboolean could_list = FALSE;
 
         if (!ensure_accounts_proxy (manager)) {
                 return;
         }
 
-        accounts_accounts_call_list_cached_users (manager->priv->accounts_proxy,
-                                                  NULL,
-                                                  on_list_cached_users_finished,
-                                                  g_object_ref (manager));
-        manager->priv->listing_cached_users = TRUE;
+        g_debug ("ActUserManager: calling 'ListCachedUsers'");
+
+        could_list = accounts_accounts_call_list_cached_users_sync (manager->priv->accounts_proxy,
+                                                                    &user_paths,
+                                                                    NULL, &error);
+
+        if (!could_list) {
+                g_debug ("ActUserManager: ListCachedUsers failed: %s", error->message);
+                g_clear_error (&error);
+                return;
+        }
+
+        load_user_paths (manager, (const char * const *) user_paths);
+        g_strfreev (user_paths);
+
+        load_included_usernames (manager);
+
+        manager->priv->list_cached_users_done = TRUE;
 }
 
 static gboolean
 load_seat_incrementally (ActUserManager *manager)
 {
         manager->priv->seat.load_idle_id = 0;
 
         switch (manager->priv->seat.state) {
         case ACT_USER_MANAGER_SEAT_STATE_GET_SESSION_ID:
                 get_current_session_id (manager);
                 break;
         case ACT_USER_MANAGER_SEAT_STATE_GET_SESSION_PROXY:
                 get_session_proxy (manager);
                 break;
         case ACT_USER_MANAGER_SEAT_STATE_GET_ID:
                 get_seat_id_for_current_session (manager);
                 break;
         case ACT_USER_MANAGER_SEAT_STATE_GET_SEAT_PROXY:
                 get_seat_proxy (manager);
                 break;
         case ACT_USER_MANAGER_SEAT_STATE_LOADED:
                 g_debug ("ActUserManager: Seat loading sequence complete");
                 break;
         default:
                 g_assert_not_reached ();
         }
 
         if (manager->priv->seat.state == ACT_USER_MANAGER_SEAT_STATE_LOADED) {
                 load_sessions (manager);
         }
 
         maybe_set_is_loaded (manager);
 
         return FALSE;
 }
 
 static gboolean
 load_idle (ActUserManager *manager)
 {
-        /* The order below is important: load_seat_incrementally might
-           set "is-loaded" immediately and we thus need to call
-           load_users before it.
-        */
-        load_users (manager);
         manager->priv->seat.state = ACT_USER_MANAGER_SEAT_STATE_UNLOADED + 1;
         load_seat_incrementally (manager);
         manager->priv->load_id = 0;
 
         return FALSE;
 }
 
 static void
-queue_load_seat_and_users (ActUserManager *manager)
+queue_load_seat (ActUserManager *manager)
 {
         if (manager->priv->load_id > 0) {
                 return;
         }
 
         manager->priv->load_id = g_idle_add ((GSourceFunc)load_idle, manager);
 }
 
 static void
 act_user_manager_get_property (GObject        *object,
                                guint           prop_id,
                                GValue         *value,
                                GParamSpec     *pspec)
 {
         ActUserManager *manager;
 
         manager = ACT_USER_MANAGER (object);
 
         switch (prop_id) {
         case PROP_IS_LOADED:
                 g_value_set_boolean (value, manager->priv->is_loaded);
                 break;
         case PROP_HAS_MULTIPLE_USERS:
                 g_value_set_boolean (value, manager->priv->has_multiple_users);
                 break;
         case PROP_INCLUDE_USERNAMES_LIST:
                 g_value_set_pointer (value, manager->priv->include_usernames);
                 break;
         case PROP_EXCLUDE_USERNAMES_LIST:
                 g_value_set_pointer (value, manager->priv->exclude_usernames);
@@ -2820,61 +2825,61 @@ act_user_manager_class_init (ActUserManagerClass *klass)
          * @user: the #ActUser that changed
          *
          * One of the users has changed
          */
         signals [USER_CHANGED] =
                 g_signal_new ("user-changed",
                               G_TYPE_FROM_CLASS (klass),
                               G_SIGNAL_RUN_LAST,
                               G_STRUCT_OFFSET (ActUserManagerClass, user_changed),
                               NULL, NULL,
                               g_cclosure_marshal_VOID__OBJECT,
                               G_TYPE_NONE, 1, ACT_TYPE_USER);
 
         g_type_class_add_private (klass, sizeof (ActUserManagerPrivate));
 }
 
 /**
  * act_user_manager_queue_load:
  * @manager: a #ActUserManager
  *
  * Queue loading users into user manager. This must be called, and the
  * #ActUserManager:is-loaded property must be %TRUE before calling
  * act_user_manager_list_users()
  */
 static void
 act_user_manager_queue_load (ActUserManager *manager)
 {
         g_return_if_fail (ACT_IS_USER_MANAGER (manager));
 
         if (! manager->priv->is_loaded) {
-                queue_load_seat_and_users (manager);
+                queue_load_seat (manager);
         }
 }
 
 static gboolean
 ensure_accounts_proxy (ActUserManager *manager)
 {
         GError *error = NULL;
 
         if (manager->priv->accounts_proxy != NULL) {
                 return TRUE;
         }
 
         manager->priv->accounts_proxy = accounts_accounts_proxy_new_sync (manager->priv->connection,
                                                                           G_DBUS_PROXY_FLAGS_NONE,
                                                                           ACCOUNTS_NAME,
                                                                           ACCOUNTS_PATH,
                                                                           NULL,
                                                                           &error);
         if (error != NULL) {
                 g_debug ("ActUserManager: getting account proxy failed: %s", error->message);
                 g_clear_error (&error);
                 return FALSE;
         }
 
         g_dbus_proxy_set_default_timeout (G_DBUS_PROXY (manager->priv->accounts_proxy), G_MAXINT);
 
         g_object_bind_property (G_OBJECT (manager->priv->accounts_proxy),
                                 "has-multiple-users",
                                 G_OBJECT (manager),
                                 "has-multiple-users",
-- 
2.14.1