[gdm/gnome-2-30] Address issues found in code review
- From: William Jon McCann <mccann src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [gdm/gnome-2-30] Address issues found in code review
- Date: Wed, 23 Jun 2010 22:28:59 +0000 (UTC)
commit ce7df502a13cfb97e5c359b4094bc716cb99b4d1
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 | 163 +++++++++++++--------------------
gui/simple-greeter/gdm-user-manager.h | 2 +-
gui/simple-greeter/gdm-user.c | 63 +++----------
gui/simple-greeter/gdm-user.h | 6 +-
4 files changed, 81 insertions(+), 153 deletions(-)
---
diff --git a/gui/simple-greeter/gdm-user-manager.c b/gui/simple-greeter/gdm-user-manager.c
index e7e0abc..c3ec109 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
+
struct GdmUserManagerPrivate
{
GHashTable *users_by_name;
@@ -567,29 +562,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;
@@ -612,23 +603,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)
@@ -647,6 +621,15 @@ add_user (GdmUserManager *manager,
g_strdup (gdm_user_get_user_name (user)),
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);
}
@@ -662,12 +645,14 @@ add_new_user_for_pwent (GdmUserManager *manager,
{
GdmUser *user;
- g_debug ("Creating new user");
+ 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);
return user;
}
@@ -676,11 +661,15 @@ 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);
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);
}
@@ -816,7 +805,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;
}
@@ -825,7 +814,8 @@ maybe_add_session (GdmUserManager *manager,
if (user == NULL) {
g_debug ("Creating new user");
- user = create_user (manager);
+ user = g_object_new (GDM_TYPE_USER, NULL);
+
_gdm_user_update_from_pwent (user, pwent);
is_new = TRUE;
} else {
@@ -843,6 +833,7 @@ maybe_add_session (GdmUserManager *manager,
/* only add the user if it was newly created */
if (is_new) {
add_user (manager, user);
+ g_object_unref (user);
}
}
@@ -1132,7 +1123,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;
@@ -1210,47 +1201,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) {
@@ -1267,12 +1223,6 @@ signal_pid (int pid,
}
}
- unblock_sigchld ();
-
- if (block_sigchld_handler < 0) {
- abort ();
- }
-
return status;
}
@@ -1525,6 +1475,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);
}
@@ -1546,12 +1501,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);
}
@@ -1615,6 +1576,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)
{
@@ -1625,8 +1603,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;
@@ -1719,7 +1697,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
@@ -1809,23 +1787,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)
@@ -2051,7 +2012,7 @@ 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);
if (manager->priv->include_all == TRUE) {
monitor_local_users (manager);
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 cc9bfa1..3ba40d5 100644
--- a/gui/simple-greeter/gdm-user.c
+++ b/gui/simple-greeter/gdm-user.c
@@ -37,9 +37,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
enum {
CHANGED,
@@ -205,7 +203,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.",
@@ -214,7 +212,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 {
@@ -229,11 +227,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;
@@ -248,11 +242,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->user_name);
user->user_name = g_strdup (pwent->pw_name);
changed = TRUE;
@@ -316,7 +306,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);
@@ -336,7 +326,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);
@@ -418,15 +408,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;
@@ -454,8 +440,7 @@ render_icon_from_cache (GdmUser *user,
gboolean res;
path = g_build_filename (GDM_CACHE_DIR, user->user_name, "face", NULL);
- res = check_user_file (path,
- MAX_FILE_SIZE);
+ res = check_user_file (path);
if (res) {
retval = gdk_pixbuf_new_from_file_at_size (path,
icon_size,
@@ -719,8 +704,7 @@ gdm_user_render_icon (GdmUser *user,
/* Try ${GlobalFaceDir}/${username} */
path = g_build_filename (GLOBAL_FACEDIR, user->user_name, NULL);
- res = check_user_file (path,
- MAX_FILE_SIZE);
+ res = check_user_file (path);
if (res) {
pixbuf = gdk_pixbuf_new_from_file_at_size (path,
icon_size,
@@ -740,8 +724,7 @@ gdm_user_render_icon (GdmUser *user,
tmp = g_strconcat (user->user_name, ".png", NULL);
path = g_build_filename (GLOBAL_FACEDIR, tmp, NULL);
g_free (tmp);
- res = check_user_file (path,
- MAX_FILE_SIZE);
+ res = check_user_file (path);
if (res) {
pixbuf = gdk_pixbuf_new_from_file_at_size (path,
icon_size,
@@ -765,32 +748,16 @@ gdm_user_render_icon (GdmUser *user,
return pixbuf;
}
-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;
}
diff --git a/gui/simple-greeter/gdm-user.h b/gui/simple-greeter/gdm-user.h
index 725c06a..f7677ce 100644
--- a/gui/simple-greeter/gdm-user.h
+++ b/gui/simple-greeter/gdm-user.h
@@ -40,12 +40,12 @@ typedef struct _GdmUser GdmUser;
GType gdm_user_get_type (void) G_GNUC_CONST;
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_primary_session_id (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]
