On 30 September 2011 20:20, Josselin Mouette <j...@debian.org> wrote: > Le vendredi 30 septembre 2011 à 15:40 +1000, dave bl a écrit : >> Ok I got it to work :) . It was fairly trivial to make it compile TM. >> The one final issue (that I encountered) was that the configuration >> stuffwasn't setting up the xephyr command :) (I have hard-coded it in >> this diff, which I know is not ok - also needing removal is the extra >> / useless debug messages that are currently in the code). > > Thanks a lot for your work. I have a few comments though.
>> /* fork X server process */ >> - res = gdm_server_spawn (server, firstserver ? "vt7" : NULL); >> - >> + res = gdm_server_spawn (server, vtarg); >> return res; >> } > > Looks like you’re using the Ubuntu patch for defining the first VT. This > won’t apply cleanly to the Debian tree. Yes, I was using the ubuntu based package. >> @@ -1010,7 +1116,9 @@ gdm_server_init (GdmServer *server) >> >> server->priv->pid = -1; >> server->priv->command = g_strdup (X_SERVER " -br -verbose"); >> + server->priv->nested_command = g_strdup ("/usr/bin/Xephyr -audit 0 >> -br"); >> server->priv->log_dir = g_strdup (LOGDIR); >> + server->priv->is_ready = FALSE; >> >> add_ready_handler (server); >> } > > I’m not really shocked to see Xephyr hardcoded. There’s not really much > point in customizing this. Well ... what if it is installed elsewhere? imho it should be a macro and defined during ./configure time :) >> - >> + > > There’s a lot of such empty changes. Please clean up your diff. Sorry, I switched my vimrc config half way through(it stripped the white-space - that's why it is a little "noisy"). I'll clean it up in a bit :) (I just wanted to get it working first :) ). >> diff --git a/daemon/gdm-slave.c b/daemon/gdm-slave.c >> index da328d3..1c5e1f5 100644 >> --- a/daemon/gdm-slave.c >> +++ b/daemon/gdm-slave.c >> @@ -522,27 +531,6 @@ gdm_slave_set_busy_cursor (GdmSlave *slave) >> } >> >> static void >> -gdm_slave_setup_xhost_auth (XHostAddress *host_entries, >> XServerInterpretedAddress *si_entries) >> -{ >> - si_entries[0].type = "localuser"; >> - si_entries[0].typelength = strlen ("localuser"); >> - si_entries[1].type = "localuser"; >> - si_entries[1].typelength = strlen ("localuser"); >> - >> - si_entries[0].value = "root"; >> - si_entries[0].valuelength = strlen ("root"); >> - si_entries[1].value = GDM_USERNAME; >> - si_entries[1].valuelength = strlen (GDM_USERNAME); >> - >> - host_entries[0].family = FamilyServerInterpreted; >> - host_entries[0].address = (char *) &si_entries[0]; >> - host_entries[0].length = sizeof (XServerInterpretedAddress); >> - host_entries[1].family = FamilyServerInterpreted; >> - host_entries[1].address = (char *) &si_entries[1]; >> - host_entries[1].length = sizeof (XServerInterpretedAddress); >> -} >> - >> -static void >> gdm_slave_set_windowpath (GdmSlave *slave) >> { >> /* setting WINDOWPATH for clients */ > > I talked about this with upstream and removing this code is a no-no. In > order to keep this the following code needs to be refactored: Right ok. What was the actual issue with the xhost code ? (I mean for nested-login you could just "drop" the xauth hostname - or did they want that to _still_ work? I would be surprised if they wanted that ... (how did gdm2.2 handle this? remote nested... :p) ). > >> @@ -633,6 +621,16 @@ gdm_slave_connect_to_x11_display (GdmSlave *slave) >> >> g_debug ("GdmSlave: Server is ready - opening display %s", >> slave->priv->display_name); >> >> + /* Nested displays are started with authorization for the parent >> + * user only. Add the GDM user. */ >> + if (slave->priv->display_is_nested) >> + { >> + g_debug ("GdmSlave: Connected to display %s", >> slave->priv->display_name); >> + g_free (slave->priv->display_x11_authority_file); >> + gdm_slave_add_user_authorization (slave, >> GDM_USERNAME, >> + >> &slave->priv->display_x11_authority_file); >> + } >> + >> g_setenv ("DISPLAY", slave->priv->display_name, TRUE); >> g_setenv ("XAUTHORITY", slave->priv->display_x11_authority_file, >> TRUE); >> >> @@ -655,25 +653,9 @@ gdm_slave_connect_to_x11_display (GdmSlave *slave) >> g_warning ("Unable to connect to display %s", >> slave->priv->display_name); >> ret = FALSE; >> } else if (slave->priv->display_is_local) { >> - XServerInterpretedAddress si_entries[2]; >> - XHostAddress host_entries[2]; >> - >> g_debug ("GdmSlave: Connected to display %s", >> slave->priv->display_name); >> ret = TRUE; >> >> - /* Give programs run by the slave and greeter access to the >> - * display independent of current hostname >> - */ >> - gdm_slave_setup_xhost_auth (host_entries, si_entries); >> - >> - gdm_error_trap_push (); >> - XAddHosts (slave->priv->server_display, host_entries, >> - G_N_ELEMENTS (host_entries)); >> - XSync (slave->priv->server_display, False); >> - if (gdm_error_trap_pop ()) { >> - g_warning ("Failed to give slave programs access to >> the display. Trying to proceed."); >> - } >> - >> gdm_slave_set_windowpath (slave); >> } else { >> g_debug ("GdmSlave: Connected to display %s", >> slave->priv->display_name); > > I had to remove the call to gdm_slave_setup_xhost_auth in order for the > nested functionality to work originally, because another call to this > function is added indirectly (I don’t exactly recall where). So this > requires moving the xhost code elsewhere. Right ok. > >> @@ -970,8 +1092,6 @@ gdm_slave_add_user_authorization (GdmSlave *slave, >> const char *username, >> char **filenamep) >> { >> - XServerInterpretedAddress si_entries[2]; >> - XHostAddress host_entries[2]; >> gboolean res; >> GError *error; >> char *filename; >> @@ -1009,19 +1129,6 @@ gdm_slave_add_user_authorization (GdmSlave *slave, >> } >> g_free (filename); >> >> - /* Remove access for the programs run by slave and greeter now that >> the >> - * user session is starting. >> - */ >> - gdm_slave_setup_xhost_auth (host_entries, si_entries); >> - gdm_error_trap_push (); >> - XRemoveHosts (slave->priv->server_display, host_entries, >> - G_N_ELEMENTS (host_entries)); >> - XSync (slave->priv->server_display, False); >> - if (gdm_error_trap_pop ()) { >> - g_warning ("Failed to remove slave program access to the >> display. Trying to proceed."); >> - } >> - >> - >> return res; >> } > > Same here. Where do you think it should go? Now I have it working, I'll revert the xhost code and see where everything breaks :p -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org