[gdm] Address issues found in code review
- From: William Jon McCann <mccann src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [gdm] Address issues found in code review
- Date: Wed, 23 Jun 2010 21:51:24 +0000 (UTC)
commit 576a4ba7b21d1426a0f9a3dd6753fbc4a33908d3
Author: William Jon McCann <jmccann redhat com>
Date: Wed Jun 23 17:50:19 2010 -0400
Address issues found in code review
https://bugzilla.gnome.org/show_bug.cgi?id=622090
gui/simple-greeter/gdm-user-manager.c | 175 ++++++++++++---------------------
gui/simple-greeter/gdm-user-manager.h | 2 +-
gui/simple-greeter/gdm-user.c | 78 +++++----------
gui/simple-greeter/gdm-user.h | 10 +-
4 files changed, 96 insertions(+), 169 deletions(-)
---
diff --git a/gui/simple-greeter/gdm-user-manager.c b/gui/simple-greeter/gdm-user-manager.c
index b8aa10b..f1592e9 100644
--- a/gui/simple-greeter/gdm-user-manager.c
+++ b/gui/simple-greeter/gdm-user-manager.c
@@ -29,8 +29,6 @@
#include <errno.h>
#include <sys/stat.h>
#include <sys/types.h>
-#include <signal.h>
-#include <errno.h>
#ifdef HAVE_PATHS_H
#include <paths.h>
@@ -52,8 +50,6 @@
#define GDM_USER_MANAGER_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), GDM_TYPE_USER_MANAGER, GdmUserManagerPrivate))
#define CK_NAME "org.freedesktop.ConsoleKit"
-#define CK_PATH "/org/freedesktop/ConsoleKit"
-#define CK_INTERFACE "org.freedesktop.ConsoleKit"
#define CK_MANAGER_PATH "/org/freedesktop/ConsoleKit/Manager"
#define CK_MANAGER_INTERFACE "org.freedesktop.ConsoleKit.Manager"
@@ -73,13 +69,12 @@
#endif
#define PATH_PASSWD "/etc/passwd"
-#define DEFAULT_GLOBAL_FACE_DIR DATADIR "/faces"
-#define DEFAULT_USER_ICON "avatar-default"
-
#ifndef GDM_USERNAME
#define GDM_USERNAME "gdm"
#endif
+#define RELOAD_PASSWD_THROTTLE_SECS 5
+
/* approximately two months */
#define LOGIN_FREQUENCY_TIME_WINDOW_SECS (60 * 24 * 60 * 60)
@@ -576,29 +571,25 @@ static gint
match_name_cmpfunc (gconstpointer a,
gconstpointer b)
{
- if (a == NULL || b == NULL) {
- return -1;
- }
-
return g_strcmp0 ((char *) a,
(char *) b);
}
static gboolean
-user_in_exclude_list (GdmUserManager *manager,
- const char *user)
+username_in_exclude_list (GdmUserManager *manager,
+ const char *username)
{
GSList *found;
gboolean ret = FALSE;
/* always exclude the "gdm" user. */
- if (user == NULL || (strcmp (user, GDM_USERNAME) == 0)) {
+ if (username == NULL || (strcmp (username, GDM_USERNAME) == 0)) {
return TRUE;
}
if (manager->priv->exclude_usernames != NULL) {
found = g_slist_find_custom (manager->priv->exclude_usernames,
- user,
+ username,
match_name_cmpfunc);
if (found != NULL) {
ret = TRUE;
@@ -621,23 +612,6 @@ add_session_for_user (GdmUserManager *manager,
g_debug ("GdmUserManager: added session for user: %s", gdm_user_get_user_name (user));
}
-static GdmUser *
-create_user (GdmUserManager *manager)
-{
- GdmUser *user;
-
- user = g_object_new (GDM_TYPE_USER, NULL);
- g_signal_connect (user,
- "sessions-changed",
- G_CALLBACK (on_user_sessions_changed),
- manager);
- g_signal_connect (user,
- "changed",
- G_CALLBACK (on_user_changed),
- manager);
- return user;
-}
-
static void
set_has_multiple_users (GdmUserManager *manager,
gboolean has_multiple_users)
@@ -665,6 +639,15 @@ add_user (GdmUserManager *manager,
g_object_ref (user));
}
+ g_signal_connect (user,
+ "sessions-changed",
+ G_CALLBACK (on_user_sessions_changed),
+ manager);
+ g_signal_connect (user,
+ "changed",
+ G_CALLBACK (on_user_changed),
+ manager);
+
if (manager->priv->is_loaded) {
g_signal_emit (manager, signals[USER_ADDED], 0, user);
}
@@ -682,14 +665,13 @@ add_new_user_for_pwent (GdmUserManager *manager,
g_debug ("GdmUserManager: Creating new user from password entry: %s", pwent->pw_name);
- user = create_user (manager);
+ user = g_object_new (GDM_TYPE_USER, NULL);
_gdm_user_update_from_pwent (user, pwent);
+ /* increases ref count */
add_user (manager, user);
g_object_unref (user);
- user = g_hash_table_lookup (manager->priv->users_by_name, pwent->pw_name);
-
return user;
}
@@ -697,6 +679,8 @@ static void
remove_user (GdmUserManager *manager,
GdmUser *user)
{
+ g_object_ref (user);
+
g_signal_handlers_disconnect_by_func (user, on_user_changed, manager);
g_signal_handlers_disconnect_by_func (user, on_user_sessions_changed, manager);
if (gdm_user_get_object_path (user) != NULL) {
@@ -705,6 +689,8 @@ remove_user (GdmUserManager *manager,
g_hash_table_remove (manager->priv->users_by_name, gdm_user_get_user_name (user));
g_signal_emit (manager, signals[USER_REMOVED], 0, user);
+ g_object_unref (user);
+
if (g_hash_table_size (manager->priv->users_by_name) > 1) {
set_has_multiple_users (manager, FALSE);
}
@@ -727,17 +713,12 @@ add_new_user_for_object_path (const char *object_path,
}
username = gdm_user_get_user_name (user);
- if (user_in_exclude_list (manager, username)) {
+ if (username_in_exclude_list (manager, username)) {
g_debug ("GdmUserManager: excluding user '%s'", username);
g_object_unref (user);
return;
}
- g_signal_connect (user, "sessions-changed",
- G_CALLBACK (on_user_sessions_changed), manager);
- g_signal_connect (user, "changed",
- G_CALLBACK (on_user_changed), manager);
-
add_user (manager, user);
g_object_unref (user);
}
@@ -758,12 +739,10 @@ on_user_removed_in_accounts_service (DBusGProxy *proxy,
gpointer user_data)
{
GdmUserManager *manager = GDM_USER_MANAGER (user_data);
- GdmUser *user;
+ GdmUser *user;
user = g_hash_table_lookup (manager->priv->users_by_object_path, object_path);
- g_object_ref (user);
remove_user (manager, user);
- g_object_unref (user);
}
static char *
@@ -990,7 +969,7 @@ maybe_add_session (GdmUserManager *manager,
g_free (x11_display);
/* check exclusions up front */
- if (user_in_exclude_list (manager, pwent->pw_name)) {
+ if (username_in_exclude_list (manager, pwent->pw_name)) {
g_debug ("GdmUserManager: excluding user '%s'", pwent->pw_name);
return;
}
@@ -1354,7 +1333,7 @@ process_ck_history_line (GdmUserManager *manager,
return;
}
- if (user_in_exclude_list (manager, username)) {
+ if (username_in_exclude_list (manager, username)) {
g_debug ("GdmUserManager: excluding user '%s'", username);
g_free (username);
return;
@@ -1422,47 +1401,12 @@ ck_history_watch (GIOChannel *source,
return TRUE;
}
-static int block_sigchld_handler = 0;
-
-static sigset_t
-block_sigchld (void)
-{
- sigset_t child_set;
- sigemptyset (&child_set);
- sigaddset (&child_set, SIGCHLD);
- sigaddset (&child_set, SIGPIPE);
- sigprocmask (SIG_BLOCK, &child_set, 0);
-
- block_sigchld_handler++;
-
- return child_set;
-}
-
-static void
-unblock_sigchld (void)
-{
- sigset_t child_set;
- sigemptyset (&child_set);
- sigaddset (&child_set, SIGCHLD);
- sigaddset (&child_set, SIGPIPE);
- sigprocmask (SIG_UNBLOCK, &child_set, 0);
-
- block_sigchld_handler--;
-}
-
static int
signal_pid (int pid,
int signal)
{
int status = -1;
- /* This function should not be called from the signal handler. */
- if (block_sigchld_handler) {
- abort ();
- }
-
- block_sigchld ();
-
status = kill (pid, signal);
if (status < 0) {
@@ -1479,12 +1423,6 @@ signal_pid (int pid,
}
}
- unblock_sigchld ();
-
- if (block_sigchld_handler < 0) {
- abort ();
- }
-
return status;
}
@@ -1745,6 +1683,11 @@ reload_passwd_file (GHashTable *valid_shells,
fclose (fp);
+ g_hash_table_iter_init (&iter, new_users_by_name);
+ while (g_hash_table_iter_next (&iter, (gpointer *) &name, (gpointer *) &user)) {
+ g_object_thaw_notify (G_OBJECT (user));
+ }
+
g_hash_table_destroy (new_users_by_name);
}
@@ -1766,12 +1709,18 @@ passwd_data_free (PasswdData *data)
g_object_unref (data->manager);
}
- g_slist_foreach (data->added_users, (GFunc) g_object_ref, NULL);
+ g_slist_foreach (data->added_users, (GFunc) g_object_unref, NULL);
g_slist_free (data->added_users);
- g_slist_foreach (data->removed_users, (GFunc) g_object_ref, NULL);
+ g_slist_foreach (data->removed_users, (GFunc) g_object_unref, NULL);
g_slist_free (data->removed_users);
+ g_slist_foreach (data->exclude_users, (GFunc) g_free, NULL);
+ g_slist_free (data->exclude_users);
+
+ g_slist_foreach (data->include_users, (GFunc) g_free, NULL);
+ g_slist_free (data->include_users);
+
g_slice_free (PasswdData, data);
}
@@ -1835,6 +1784,23 @@ do_reload_passwd_job (GIOSchedulerJob *job,
return FALSE;
}
+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
schedule_reload_passwd (GdmUserManager *manager)
{
@@ -1845,8 +1811,8 @@ schedule_reload_passwd (GdmUserManager *manager)
passwd_data = g_slice_new0 (PasswdData);
passwd_data->manager = g_object_ref (manager);
passwd_data->shells = manager->priv->shells;
- passwd_data->exclude_users = manager->priv->exclude_usernames;
- passwd_data->include_users = manager->priv->include_usernames;
+ passwd_data->exclude_users = slist_deep_copy (manager->priv->exclude_usernames);
+ passwd_data->include_users = slist_deep_copy (manager->priv->include_usernames);
passwd_data->include_all = manager->priv->include_all;
passwd_data->current_users_by_name = manager->priv->users_by_name;
passwd_data->added_users = NULL;
@@ -1998,7 +1964,7 @@ queue_reload_passwd (GdmUserManager *manager)
g_source_remove (manager->priv->reload_passwd_id);
}
- manager->priv->reload_passwd_id = g_timeout_add_seconds (5, (GSourceFunc)reload_passwd_idle, manager);
+ manager->priv->reload_passwd_id = g_timeout_add_seconds (RELOAD_PASSWD_THROTTLE_SECS, (GSourceFunc)reload_passwd_idle, manager);
}
static void
@@ -2064,23 +2030,6 @@ gdm_user_manager_get_property (GObject *object,
}
}
-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
set_include_usernames (GdmUserManager *manager,
GSList *list)
@@ -2289,12 +2238,12 @@ gdm_user_manager_init (GdmUserManager *manager)
manager->priv->users_by_name = g_hash_table_new_full (g_str_hash,
g_str_equal,
g_free,
- (GDestroyNotify) g_object_run_dispose);
+ g_object_unref);
manager->priv->users_by_object_path = g_hash_table_new_full (g_str_hash,
g_str_equal,
NULL,
- (GDestroyNotify) g_object_unref);
+ g_object_unref);
g_assert (manager->priv->seat_proxy == NULL);
@@ -2353,6 +2302,10 @@ gdm_user_manager_finalize (GObject *object)
g_object_unref (manager->priv->seat_proxy);
}
+ if (manager->priv->accounts_proxy != NULL) {
+ g_object_unref (manager->priv->accounts_proxy);
+ }
+
if (manager->priv->ck_history_id != 0) {
g_source_remove (manager->priv->ck_history_id);
manager->priv->ck_history_id = 0;
diff --git a/gui/simple-greeter/gdm-user-manager.h b/gui/simple-greeter/gdm-user-manager.h
index 116e3fb..96c2284 100644
--- a/gui/simple-greeter/gdm-user-manager.h
+++ b/gui/simple-greeter/gdm-user-manager.h
@@ -53,7 +53,7 @@ typedef struct
void (* user_is_logged_in_changed) (GdmUserManager *user_manager,
GdmUser *user);
void (* user_changed) (GdmUserManager *user_manager,
- GdmUser *user);
+ GdmUser *user);
} GdmUserManagerClass;
typedef enum
diff --git a/gui/simple-greeter/gdm-user.c b/gui/simple-greeter/gdm-user.c
index abecc65..8227ff8 100644
--- a/gui/simple-greeter/gdm-user.c
+++ b/gui/simple-greeter/gdm-user.c
@@ -40,9 +40,7 @@
#define GDM_USER_GET_CLASS(object) (G_TYPE_INSTANCE_GET_CLASS ((object), GDM_TYPE_USER, GdmUserClass))
#define GLOBAL_FACEDIR DATADIR "/faces"
-#define MAX_ICON_SIZE 128
#define MAX_FILE_SIZE 65536
-#define MINIMAL_UID 100
#define USER_ACCOUNTS_NAME "org.freedesktop.Accounts"
#define USER_ACCOUNTS_INTERFACE "org.freedesktop.Accounts.User"
@@ -74,8 +72,7 @@ typedef struct _GdmUserClass
} GdmUserClass;
static void gdm_user_finalize (GObject *object);
-static gboolean check_user_file (const char *filename,
- gssize max_file_size);
+static gboolean check_user_file (const char *filename);
static guint signals[LAST_SIGNAL] = { 0 };
@@ -192,6 +189,7 @@ gdm_user_finalize (GObject *object)
g_free (user->user_name);
g_free (user->real_name);
g_free (user->icon_file);
+ g_free (user->object_path);
if (user->accounts_proxy != NULL) {
g_object_unref (user->accounts_proxy);
@@ -234,7 +232,7 @@ _gdm_user_update_from_pwent (GdmUser *user,
if (g_utf8_validate (pwent->pw_gecos, -1, NULL)) {
valid_utf8_name = pwent->pw_gecos;
- first_comma = g_utf8_strchr (valid_utf8_name, -1, ',');
+ first_comma = strchr (valid_utf8_name, ',');
} else {
g_warning ("User %s has invalid UTF-8 in GECOS field. "
"It would be a good thing to check /etc/passwd.",
@@ -243,7 +241,7 @@ _gdm_user_update_from_pwent (GdmUser *user,
if (first_comma) {
real_name = g_strndup (valid_utf8_name,
- (first_comma - valid_utf8_name));
+ (first_comma - valid_utf8_name));
} else if (valid_utf8_name) {
real_name = g_strdup (valid_utf8_name);
} else {
@@ -258,11 +256,7 @@ _gdm_user_update_from_pwent (GdmUser *user,
real_name = NULL;
}
- if ((real_name && !user->real_name) ||
- (!real_name && user->real_name) ||
- (real_name &&
- user->real_name &&
- strcmp (real_name, user->real_name) != 0)) {
+ if (g_strcmp0 (real_name, user->real_name) != 0) {
g_free (user->real_name);
user->real_name = real_name;
changed = TRUE;
@@ -277,12 +271,7 @@ _gdm_user_update_from_pwent (GdmUser *user,
}
/* Username */
- if ((pwent->pw_name && !user->user_name) ||
- (!pwent->pw_name && user->user_name) ||
- (pwent->pw_name &&
- user->user_name &&
- strcmp (user->user_name, pwent->pw_name) != 0)) {
-
+ if (g_strcmp0 (pwent->pw_name, user->user_name) != 0) {
g_free (user->icon_file);
user->icon_file = NULL;
if (pwent->pw_name != NULL) {
@@ -290,9 +279,7 @@ _gdm_user_update_from_pwent (GdmUser *user,
user->icon_file = g_build_filename (GDM_CACHE_DIR, pwent->pw_name, "face", NULL);
- res = check_user_file (user->icon_file,
- MAX_FILE_SIZE);
-
+ res = check_user_file (user->icon_file);
if (!res) {
g_free (user->icon_file);
user->icon_file = g_build_filename (GLOBAL_FACEDIR, pwent->pw_name, NULL);
@@ -362,7 +349,7 @@ gdm_user_get_uid (GdmUser *user)
*
* Since: 1.0
**/
-G_CONST_RETURN gchar *
+const char *
gdm_user_get_real_name (GdmUser *user)
{
g_return_val_if_fail (GDM_IS_USER (user), NULL);
@@ -382,7 +369,7 @@ gdm_user_get_real_name (GdmUser *user)
* Since: 1.0
**/
-G_CONST_RETURN gchar *
+const char *
gdm_user_get_user_name (GdmUser *user)
{
g_return_val_if_fail (GDM_IS_USER (user), NULL);
@@ -464,15 +451,11 @@ gdm_user_collate (GdmUser *user1,
}
static gboolean
-check_user_file (const char *filename,
- gssize max_file_size)
+check_user_file (const char *filename)
{
+ gssize max_file_size = MAX_FILE_SIZE;
struct stat fileinfo;
- if (max_file_size < 0) {
- max_file_size = G_MAXSIZE;
- }
-
/* Exists/Readable? */
if (stat (filename, &fileinfo) < 0) {
return FALSE;
@@ -732,8 +715,7 @@ gdm_user_render_icon (GdmUser *user,
pixbuf = NULL;
if (user->icon_file) {
- res = check_user_file (user->icon_file,
- MAX_FILE_SIZE);
+ res = check_user_file (user->icon_file);
if (res) {
pixbuf = gdk_pixbuf_new_from_file_at_size (user->icon_file,
icon_size,
@@ -772,7 +754,7 @@ gdm_user_render_icon (GdmUser *user,
return pixbuf;
}
-G_CONST_RETURN gchar *
+const char *
gdm_user_get_icon_file (GdmUser *user)
{
g_return_val_if_fail (GDM_IS_USER (user), NULL);
@@ -780,7 +762,7 @@ gdm_user_get_icon_file (GdmUser *user)
return user->icon_file;
}
-G_CONST_RETURN gchar *
+const char *
gdm_user_get_object_path (GdmUser *user)
{
g_return_val_if_fail (GDM_IS_USER (user), NULL);
@@ -788,33 +770,17 @@ gdm_user_get_object_path (GdmUser *user)
return user->object_path;
}
-G_CONST_RETURN char *
+const char *
gdm_user_get_primary_session_id (GdmUser *user)
{
- GList *l;
- const char *primary_ssid;
-
- primary_ssid = NULL;
-
if (!gdm_user_is_logged_in (user)) {
g_debug ("User %s is not logged in, so has no primary session",
gdm_user_get_user_name (user));
- goto out;
+ return NULL;
}
- for (l = user->sessions; l != NULL; l = l->next) {
- const char *ssid;
-
- ssid = l->data;
-
- /* FIXME: better way to choose? */
- if (ssid != NULL) {
- primary_ssid = ssid;
- break;
- }
- }
-out:
- return primary_ssid;
+ /* FIXME: better way to choose? */
+ return user->sessions->data;
}
static void
@@ -843,8 +809,16 @@ collect_props (const gchar *key,
} else if (strcmp (key, "LoginFrequency") == 0) {
user->login_frequency = g_value_get_uint64 (value);
} else if (strcmp (key, "IconFile") == 0) {
+ gboolean res;
+
g_free (user->icon_file);
user->icon_file = g_value_dup_string (value);
+
+ res = check_user_file (user->icon_file);
+ if (!res) {
+ g_free (user->icon_file);
+ user->icon_file = g_build_filename (GLOBAL_FACEDIR, user->user_name, NULL);
+ }
} else if (strcmp (key, "Locked") == 0) {
/* ignore */
} else if (strcmp (key, "AutomaticLogin") == 0) {
diff --git a/gui/simple-greeter/gdm-user.h b/gui/simple-greeter/gdm-user.h
index ff566a7..2aee30a 100644
--- a/gui/simple-greeter/gdm-user.h
+++ b/gui/simple-greeter/gdm-user.h
@@ -39,17 +39,17 @@ typedef struct _GdmUser GdmUser;
GType gdm_user_get_type (void) G_GNUC_CONST;
-GdmUser *gdm_user_new_from_object_path (const gchar *path);
+GdmUser *gdm_user_new_from_object_path (const char *path);
const char *gdm_user_get_object_path (GdmUser *user);
gulong gdm_user_get_uid (GdmUser *user);
-G_CONST_RETURN char *gdm_user_get_user_name (GdmUser *user);
-G_CONST_RETURN char *gdm_user_get_real_name (GdmUser *user);
+const char *gdm_user_get_user_name (GdmUser *user);
+const char *gdm_user_get_real_name (GdmUser *user);
guint gdm_user_get_num_sessions (GdmUser *user);
gboolean gdm_user_is_logged_in (GdmUser *user);
gulong gdm_user_get_login_frequency (GdmUser *user);
-G_CONST_RETURN char *gdm_user_get_icon_file (GdmUser *user);
-G_CONST_RETURN char *gdm_user_get_primary_session_id (GdmUser *user);
+const char *gdm_user_get_icon_file (GdmUser *user);
+const char *gdm_user_get_primary_session_id (GdmUser *user);
GdkPixbuf *gdm_user_render_icon (GdmUser *user,
gint icon_size);
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
