Bug#643634: restore nested login in gdm3

[dave bl]

On 30 September 2011 20:20, Josselin Mouette <joss at 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