Re: [gdm-list] [PATCH] fix race condition when setting the model for the face browser

[Brian Cameron <Brian Cameron Sun COM>]

Ludwig:

Thanks for the patch.  I committed this to HEAD and 2.16 branch.  This
does look much better.

> Ok, so this was driving me nuts the whole day until I understood
> what's going on. r4381 changed the way how the face browser gets
> constructed. Previously the treeview got an empty model and then the
> model was populated with user names. Now the model is populated
> first and then attached to the treeview. Looks like this causes the
> selection's "changed" signal to be emmited which in turn triggers an
> interruption message to the slave process. It is treated as a reply
> to GDM_NEEDPIC there. Sometimes (race condition?) the real reply for
> GDM_NEEDPIC then ends up as reply for the username prompt which
> causes the first pam conversation to fail.
>
> Attached patch fixes that by not populating the model before it has
> been attached to the treeview again.
>
> While debuggint that problem I saw two problems:
>
> - There is no code in place that prevents interruption messages
>   (those with BEL) at the wrong time.

This is a problem.  If you aren't able to find time to fix this,
then please file a bug at bugzilla.gnome.org in the gdm2 category
so the problem is not forgotten about.

> - The way pictures are transported into the greeter is really ugly.
>   IMHO the greeter could just as well open the files itself. There
>   is no need to let the slave process do it.

Yes it is ugly.  The problem is that the GUI runs as the gdm user
and the gdm user does not necessarily have read access to user's
$HOME directory.  The daemon runs as root and has the ability to
read image files without these sorts of problems.

Probably a better mechanism for passing the images from the daemon
to the GUI could be devised.  Perhaps the daemon should copy them to
a location where the GUI can find them (somewhere like /var/tmp/gdm)
rather than sending them over the sockets connection.

Note GDM can be configured to use GlobalFaceDir instead of reading
the image files from the user's $HOME directory, but even in this
case I think the daemon dumbly sends the images over the wire rather
than having the GUI just access the images from there directly.

Again, please file a bug if you don't think you will work on fixing
this.

Brian


> ------------------------------------------------------------------------
>
> Index: gdm2/gui/gdmlogin.c
> ===================================================================
> --- gdm2.orig/gui/gdmlogin.c
> +++ gdm2/gui/gdmlogin.c
> @@ -1999,17 +1999,11 @@ process_operation (guchar       op_code,
>  }
>  
>  
> -static int
> +static void
>  gdm_login_browser_populate (void)
>  {
>      GList *li;
> -    int i = 0;
>  
> -    /* Create browser_model before calling gdm_login_browser_populate */
> -       browser_model = (GtkTreeModel *)gtk_list_store_new (3,
> -							   GDK_TYPE_PIXBUF,
> -							   G_TYPE_STRING,
> -							   G_TYPE_STRING);
>      for (li = users; li != NULL; li = li->next) {
>  	    GdmUser *usr = li->data;
>  	    GtkTreeIter iter = {0};
> @@ -2032,9 +2026,8 @@ gdm_login_browser_populate (void)
>  				GREETER_ULIST_LABEL_COLUMN, label,
>  				-1);
>  	    g_free (label);
> -	    i++;
>      }
> -    return (i);
> +    return;
>  }
>  
>  static void
> @@ -2592,6 +2585,12 @@ gdm_login_gui_init (void)
>  			      G_CALLBACK (browser_change_focus),
>  			      NULL);
>  
> +	    /* Create browser_model before calling gdm_login_browser_populate */
> +	   browser_model = (GtkTreeModel *)gtk_list_store_new (3,
> +							   GDK_TYPE_PIXBUF,
> +							   G_TYPE_STRING,
> +							   G_TYPE_STRING);
> +
>  	    gtk_tree_view_set_model (GTK_TREE_VIEW (browser), browser_model);
>  	    column = gtk_tree_view_column_new_with_attributes
>  		    (_("Icon"),
> @@ -3574,18 +3573,16 @@ main (int argc, char *argv[])
>  	    }
>      }
>  
> -    if (browser_ok && gdm_config_get_bool (GDM_KEY_BROWSER)) {
> -	/*
> -	 * Do not display face browser widget if no users, check this
> -	 * before callign gdm_login_gui_init ()
> -	 */
> -	int num_users = gdm_login_browser_populate ();
> -	if (num_users == 0)
> -		browser_ok = FALSE;
> -    }
> +    /* Do not display face browser widget if no users */
> +    if(!users)
> +	browser_ok = FALSE;
>  
>      gdm_login_gui_init ();
>  
> +    if (browser_ok && gdm_config_get_bool (GDM_KEY_BROWSER)) {
> +	gdm_login_browser_populate ();
> +    }
> +
>      ve_signal_add (SIGHUP, gdm_reread_config, NULL);
>  
>      hup.sa_handler = ve_signal_notify;
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> gdm-list mailing list
> gdm-list gnome org
> http://mail.gnome.org/mailman/listinfo/gdm-list