Re: [gdm-list] Reworked patch for multi-seat and multi-display support

[Ray Strode <halfline gmail com>]

Hi,

On Thu, Jun 4, 2009 at 10:57 PM, Halton Huo<Halton Huo sun com> wrote:
> I've finished the tasks below and now the new patch is ready for review.
I've posted a reply to the ConsoleKit chunk of work here:

http://lists.freedesktop.org/archives/consolekit/2009-June/000005.html

I'll use this email to talk about the gdm bits.

> diff --git a/daemon/gdm-dynamic-display.c b/daemon/gdm-dynamic-display.c
> new file mode 100644
> index 0000000..ea07b1d
> --- /dev/null
> +++ b/daemon/gdm-dynamic-display.c
[...]
> +static gboolean
> +gdm_dynamic_display_finish (GdmDisplay *display)
> +{
> +        int status;
> +
> +        g_return_val_if_fail (GDM_IS_DISPLAY (display), FALSE);
> +
> +        /* restart dynamic display */
> +        gdm_display_unmanage (display);
> +
> +        status = gdm_display_get_status (display);
> +        if (status != GDM_DISPLAY_FAILED) {
> +                gdm_display_manage (display);
> +        }
> +
> +        return TRUE;
> +}
This bit is copied from gdm-static-display but it doesn't make sense
for a dynamic display.

You should just chain up to the default finish handler, and let
consolekit/the local factory handle
restarting the display.

> diff --git a/daemon/gdm-local-display-factory.c b/daemon/gdm-local-display-factory.c
> index 3984ada..72d2816 100644
> --- a/daemon/gdm-local-display-factory.c
> +++ b/daemon/gdm-local-display-factory.c
> @@ -53,11 +61,15 @@
>
>  #define MAX_DISPLAY_FAILURES 5
>
> +#define IS_STR_SET(x) (x != NULL && x[0] != '\0')
> +
>  struct GdmLocalDisplayFactoryPrivate
>  {
>          DBusGConnection *connection;
> -        DBusGProxy      *proxy;
> +        DBusGProxy      *proxy_hal;
> +        DBusGProxy      *proxy_ck;
>          GHashTable      *displays;
> +        GSList          *lst_proxy;
what does lst_proxy mean? Proxy list? I'd,

1) Call it seat_proxies;
2) Make it a hash table instead of a list so you can get at seats
from their ids.

[...]
> +static GdmDisplay *
> +gdm_local_display_lookup_by_number (GdmLocalDisplayFactory *factory,
> +                                    gint32                  number)
> +{
> +        GdmDisplay      *display;
> +        GdmDisplayStore *store;
> +
> +        if (number < 0)
> +                return NULL;
> +
> +        store = gdm_display_factory_get_display_store (GDM_DISPLAY_FACTORY (factory));
> +        display = gdm_display_store_find (store,
> +                                          (GdmDisplayStoreFunc)lookup_by_display_number,
> +                                          GINT_TO_POINTER (number));
> +
> +        return display;
> +}
> +
This is a little icky, would be better if this look up were constant time.

On the other hand, this function shouldn't be needed with the
ConsoleKit changes I proposed.
> +#ifndef HAVE_STRREP
> +static void
> +strrep (char* in, char** out, char* old, char* new)
> +{
> +        char* temp;
> +        char* orig = strdup(in);
> +        char* found = strstr(orig, old);
> +        if(!found) {
> +                *out = malloc(strlen(orig) + 1);
> +                strcpy(*out, orig);
> +                return;
> +        }
> +
> +        int idx = found - orig;
> +
> +        *out = realloc(*out, strlen(orig) - strlen(old) + strlen(new) + 1);
> +        strncpy(*out, orig, idx);
> +        strcpy(*out + idx, new);
> +        strcpy(*out + idx + strlen(new), orig + idx + strlen(old));
> +
> +
> +        temp = malloc(idx+strlen(new)+1);
> +        strncpy(temp,*out,idx+strlen(new));
> +        temp[idx + strlen(new)] = '\0';
> +
> +        strrep(found + strlen(old), out, old, new);
> +        temp = realloc(temp, strlen(temp) + strlen(*out) + 1);
> +        strcat(temp,*out);
> +        free(*out);
> +        *out = temp;
> +}
> +#endif
Again (just like in the consolekit patch) this could probably be replaced with
a g_regex call

> +
> +static void
> +seat_session_to_add (DBusGProxy             *seat_proxy,
> +                     gboolean                is_dynamic,
> +                     const char             *xserver_command,
> +                     GdmLocalDisplayFactory *factory)
> +{
The signature of this had to get changed to accomodate the new consolekit api.
It now passes what kind of session type is expected ("LoginWindow" or
"Chooser" or whatever), a dictionary of variables like display number,
vt, etc and the type of display, e.g., "X11"
[...]
> +        comm = g_strdup (xserver_command);
> +        for (i = 0; i < argc; i++) {
> +                /* replase $display in case of not specified */
> +                if (g_str_equal (argv[i], "$display")) {
> +                        display_number = take_next_display_number (factory);
> +                        strrep (comm, &comm, "$display", "");
> +                        break;
> +                }
> +
> +                /* get display_number in case of specified */
> +                if (g_str_has_prefix (argv[i], ":")) {
> +                        display_number = atoi (argv[i]+1);
> +                        strrep (comm, &comm, argv[i], "");
> +                        break;
> +                }
> +        }
> +        g_strfreev (argv);
> +
> +        is_chooser = FALSE;
> +        if (strstr (comm, "-indirect")) {
> +                is_chooser = TRUE;
> +        }
> +
> +        use_auth = FALSE;
> +        if (strstr (comm, "-auth $auth")) {
> +                use_auth = TRUE;
> +                strrep (comm, &comm, "-auth $auth", "");
> +        }
So what you're doing here is stripping parts of the command line out
and settings flags etc and then making lower layers in the code
rebuild the parts that you stripped out.

I think a better approach would be to hand the command line down
wholesale along with the list of variables passed in from the signal.
> +
> +        if (is_chooser) {
> +                /* TODO: Start a xdmcp chooser as request */
> +
> +                /* display = gdm_xdmcp_chooser_display_new (display_number); */
We need to flesh this part out.
> +        } else {
> +                if (is_dynamic)
> +                        display = gdm_dynamic_display_new (display_number);
> +                else
> +                        display = gdm_static_display_new (display_number);
We should probably only use gdm_dynamic_display_new here.  All the
displays coming from ConsoleKit are coming "on the fly" and could
disappear on the fly.  I can't think of a good reason to special case
dynamic displays.

That means we should probably drop the whole is-dynamic bits from
ConsoleKit entirely.
> +static void
> +seat_session_to_remove (DBusGProxy             *seat_proxy,
> +                        int                     display_number,
> +                        GdmLocalDisplayFactory *factory)
> +{
[...]
> +}
This function needed to get updated to use the new CK proposal.
> +
> +static void
> +marshal_VOID__BOOLEAN_STRING (GClosure     *closure,
> +                              GValue       *return_value,
> +                              guint         n_param_values,
> +                              const GValue *param_values,
> +                              gpointer      invocation_hint,
> +                              gpointer      marshal_data)
> +{
[...]
> +}
Putting goo like this in the source file isn't very nice.  marshalers
are automatically generated in the common directory. You just need to
add them to the .list file and include gdm-marshal.h

Other issues:

- We need to take the session id passed to SessionToAdd and bubble it
through the layers of code, so that when GDM calls
OpenSessionWithParameters it passes it back to ConsoleKit
- When a display changes which session is running on it (for instance
when going from the login screen to the user session) we need to
update the session id on the display
- We need a way to tell the local factory to ignore seat session
requests momentarily after closing the login window session before the
user's session is started.
- we should drop create_display since it's not used anymore
- Need to handle $vt on the command line

I've done some of these changes on the display-configuration branch here:

http://www.gnome.org/~halfline/gdm

(also available for reading as a patch here:
http://www.gnome.org/~halfline/gdm-display-configuration.patch)

--Ray