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;

         }