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

[Ray Strode <halfline gmail com>]

Hi,

On Thu, Apr 9, 2009 at 7:02 AM, Halton Huo <Halton Huo sun com> wrote:
> Hi,
>
> Here comes the patches for multi-seat and multi-display support, please
> review:
>
> * https://bugs.freedesktop.org/attachment.cgi?id=24683
>  ConsoleKit part based on ConsoleKit-0.3.0 tarball release

I've checked you're ConsoleKit patch onto the 'multi-seat' branch in
ConsoleKit git while we work on it:

http://cgit.freedesktop.org/ConsoleKit/log/?h=multi-seat

and your gdm patch onto the 'display-configuration' branch in gdm git:

http://git.gnome.org/cgit/gdm/log/?h=display-configuration

Here's a review of the ConsoleKit bits, I'll hold off on the GDM
review until we've discussed the ConsoleKit
changes a bit (since changes to ConsoleKit will imply changes to GDM).

> --- a/configure.ac
> +++ b/configure.ac
> @@ -175,6 +175,7 @@ dnl ---------------------------------------------------------------------------
>
>  CK_BACKEND=""
>  KVM_LIBS=""
> +X11_DIR="/usr/bin"
>  case "$host" in
>          *-*-freebsd*)
>          CK_BACKEND="freebsd"
> @@ -189,11 +190,13 @@ case "$host" in
>          ;;
>          *-*-solaris*)
>          CK_BACKEND="solaris"
> +        X11_DIR="/usr/X11/bin"
>          ;;
>  	*)
>  	AC_MSG_ERROR([No sysdeps back-end implemented for host $host])
>  	;;
>  esac
> +AC_SUBST(X11_DIR)
>
>  AC_SUBST(KVM_LIBS)

Can you figure out X11_DIR from pkg-config?

> diff --git a/data/00-primary.seat b/data/00-primary.seat
> index 6e61db4..7362125 100644
> --- a/data/00-primary.seat
> +++ b/data/00-primary.seat
> @@ -1,5 +1,34 @@
>  [Seat Entry]
> -Version=1.0
> -Name=Primary seat
> +Description=start one static local display at :0
> +
I wouldn't get rid of the Name even if you aren't using it explicitly.
It was already there after all and we might use it at some point in the future.

Same with Version.  I feel like it should still be there.  Given we
weren't using this file before we can probably keep it at 1.0.

>  Hidden=false
> +# Indicate whether to create this seat or not. If it is set true, then CK will
> +# not create this seat. Default value is false.
> +
> +DisplayType=Local
> +# Indicate display type which defined in <etc>/ConsoleKit/displays-types.conf

Comments should appear before the lines they're commenting on, not after.
> +DisplayNumbers=0
> +# Display numbers list, separated by a separator character, typically ';'
> +# or ','. If want to automatically choose next available display number,
> +# indicate -1
I wouldn't allow ';' and ','.  I would just pick one (and I would pick ';').
The only reason gkeyfile let's you pick, is because when the icon theme spec
got written no one noticed that desktop files used ; instead of , so it ended
up getting the wrong one (and when gkeyfile got written we wanted it to work
for the icon theme files too)

So if I want to configure my system to start two displays at boot up, with
any display number, I don't care, is it:

DisplayNumbers=-1;-1;

?  That's a little clunky.

> +
> +TtyDevices=
> +# Values list to indicate the tty device number, separated by a separator
> +# character, typically ';' or ','.
> +# Zero or negative number or no value means choose next available vt number
So TtyDevices and DisplayNumbers have to always be in sync?  This
seems very fragile to changes.

I think we need a separate [Display Entry] that gets tied to a
particular seat. Then in the .seat file we'd have

Displays=localdisplay1;localdisplay2;

or some such.  The display entry file would have what display number
to use and what tty to use
(and the absense of those keys would mean "figure it out automatically")

DisplayType could get moved to the display entry, too.  Or maybe we
requires all displays at one seat to have the same type, not sure.

> +
> +IsConsoles=true
> +# Values list to indicate whether the give DisplayNumbers are on console
> +# or not , separated by a separator character, typically ';' or ','.
That shouldn't be plural.  Maybe "RequiresConsole" would be better?
If we end up with Displays= above, then we'll need to make sure that
all the display types in the list
match in the "RequiresConsole" respect.

> diff --git a/data/display-types.conf.in b/data/display-types.conf.in
> new file mode 100644
> index 0000000..4c4246d
> --- /dev/null
> +++ b/data/display-types.conf.in
> @@ -0,0 +1,27 @@
> +[Local]
> +Command= X11_DIR@/Xorg
> +Arguments=-br -verbose -nolisten tcp
> +Chooser=false
> +UseAuth=true
> +Priority=
Why is Command and Arguments two separate things?  I'd do this as

Command= X11_DIR@/Xorg $display -br -verbose -nolisten tcp $vt

Then $display and $vt would get substituted with whatever display is
specified in the seat display configuration.

> +
> +[RemoteMachine]
> +Command= X11_DIR@/Xorg
> +Arguments=-br -verbose -indirect
> +Chooser=true
> +UseAuth=true
> +Priority=
> +
> +[LocalVNC]
> +Command= X11_DIR@/Xvnc
> +Arguments=-query localhost
> +Chooser=false
> +UseAuth=true
> +Priority=
> +
> +[Headless]
> +Command= X11_DIR@/Xvfb
> +Chooser=false
> +Arguments=
> +UseAuth=true
> +Priority=

I'm a little worried that these guys are all getting stuffed into one file.
Who "owns" the file, the distribution or the sysadmin?  If the distro decides
to change which command line arguments are passed to the X server is it going
to have to tell the sysadmin to update the file?

If you did each entry as a separate file, like you did for seat configuration,
this will be a lot more managable.  E.g.,

[Display Type]
Name=Local
Description=Configuration to use for local displays
Command=Xorg $display -br -verbose -nolisten tcp $vt
Chooser=false
UseAuth=true

> --- /dev/null
> +++ b/src/ck-display-type.c
> @@ -0,0 +1,425 @@
[...]
> +#if __sun
> +#define CK_PRIO_MIN 0
> +#define CK_PRIO_MAX (NZERO*2)-1
> +#define CK_PRIO_DEFAULT NZERO
> +#else
> +#include <sys/resource.h>
> +#define CK_PRIO_MIN PRIO_MIN
> +#define CK_PRIO_MAX PRIO_MAX
> +#define CK_PRIO_DEFAULT 0
> +#endif
This bit is a little ugly. If priority makes sense to expose at all
(which I'm not sure about), it should go
over the wire to display manager in a standard range that isn't os specific.
Maybe a double in the range 0.0 to 1.0 ?

Alternatively, maybe we should just have a flag "runs high priority" or
whatever.  I don't understand the use case for priority, so I don't know the
answer to this question: "Do values other than minimum, maximum, and normal
ever make sense to use?"

[...]
> +static void
> +_ck_display_type_set_priority (CkDisplayType  *display,
> +                               int             priority)
> +{
> +        /* do some bounds checking */
> +        if (priority < CK_PRIO_MIN)
> +                display->priv->priority = CK_PRIO_MIN;
> +        else if (priority > CK_PRIO_MAX)
> +                display->priv->priority = CK_PRIO_MAX;
> +        else
> +                display->priv->priority = priority;
> +}
g_param_spec_int (which you used when creating the property) let's you
enforce value ranges.

[...]
> +static void
> +ck_display_type_init (CkDisplayType *display)
> +{
> +        display->priv = CK_DISPLAY_TYPE_GET_PRIVATE (display);
> +
> +        display->priv->name = NULL;
> +        display->priv->command = NULL;
> +        display->priv->arguments = NULL;
> +        display->priv->use_auth = TRUE;
> +        display->priv->priority = 0;
Shouldn't this be CK_PRIO_DEFAULT ?
[...]
> +CkDisplayType *
> +ck_display_type_new (const char *name)
> +{
> +        GObject   *object;
> +        GKeyFile  *key_file;
> +        char     **groups;
> +        gsize      group_len;
> +        gsize      i;
> +        gboolean   res;
> +        GError    *error;
> +
> +        g_return_val_if_fail ((name && !g_str_equal (name, "")), NULL);
> +
> +        key_file = g_key_file_new ();
> +
> +        error = NULL;
> +        res = g_key_file_load_from_file (key_file,
> +                                         CK_DISPLAY_TYPES_FILE,
> +                                         G_KEY_FILE_NONE,
> +                                         &error);
> +        if (! res) {
> +                g_warning ("Unable to load static display types from file %s: %s", CK_DISPLAY_TYPES_FILE, error->message);
> +                g_error_free (error);
> +                return NULL;
> +        }
> +
Two things that are generally considered bad practice:

    1) Doing things that can fail at object construction time.  Don't
ever return NULL from a _new function.
    2) Making _new() more than a 3 line wrapper around g_object_new ().

I would have a separate _load () function that returns a gerror.

> +        groups = g_key_file_get_groups (key_file, &group_len);
> +        for(i=0; i<group_len; i++) {
> +                if (g_str_equal (groups[i], name)) {
This seems really clunky to me.  It means the file could conceivably get
opened, read, and closed several times at startup.

I really think that each display type should be in its own file.  If you
enforce a rule that each file be named with the same name as it's internal
name, then you can just open "Local.display" or whatever for the local display
type and don't have to do any searching or rereading.

[...]
> diff --git a/src/ck-manager.c b/src/ck-manager.c
> index bcb9350..f95c8c5 100644
> --- a/src/ck-manager.c
> +++ b/src/ck-manager.c
> @@ -49,6 +49,7 @@
>  #include "ck-manager.h"
>  #include "ck-manager-glue.h"
>  #include "ck-seat.h"
> +#include "ck-display-type.h"
>  #include "ck-session-leader.h"
>  #include "ck-session.h"
>  #include "ck-marshal.h"
> @@ -97,6 +98,15 @@ static guint signals [LAST_SIGNAL] = { 0, };
>  static void     ck_manager_class_init  (CkManagerClass *klass);
>  static void     ck_manager_init        (CkManager      *manager);
>  static void     ck_manager_finalize    (GObject        *object);
> +static gboolean create_display_with_parameters (CkManager *manager,
> +                                                gint32     display_number,
> +                                                gint32     seat_number,
> +                                                char      *display_type,
> +                                                gint32     priority,
> +                                                gboolean   is_console,
> +                                                char      *tty_device,
> +                                                gboolean   is_dynamic,
> +                                                char      **id);
This prototype declares a function that's never defined...
[...]
> +static void
> +_create_static_displays_per_seat (gpointer key,
> +                                  gpointer value,
> +                                  gpointer user_data)
> +{
> +        ck_seat_create_static_sessions (CK_SEAT (value));
> +}
> +
> +/*
> +  Example:
> +  dbus-send --system --dest=org.freedesktop.ConsoleKit \
> +  --type=method_call --print-reply --reply-timeout=2000 \
> +  /org/freedesktop/ConsoleKit/Manager \
> +  org.freedesktop.ConsoleKit.Manager.CreateStaticSessions
> +*/
> +gboolean
> +ck_manager_create_static_sessions (CkManager             *manager,
> +                                   DBusGMethodInvocation *context)
> +{
> +        g_hash_table_foreach (manager->priv->seats, _create_static_displays_per_seat, NULL);
> +        return TRUE;
> +}
> +
So this CreateStaticSessions function is supposed to be called by the
display manager to make
ConsoleKit tell the display manager what seat config to instantiate?
I don't think the name is right.

Thinking about it, we have a bunch of seats, and each seat has a bunch
of sessions.

Maybe the right interface is for the display manager to first

Manager.GetUnmanagedSeats

then for each seat, the display manager would "opt-in" via
ManageSeat() function.

When that function gets called ConsoleKit would emit a signal for each session
in the seat that needs to be started.
>  gboolean
>  ck_manager_open_session (CkManager             *manager,
>                           DBusGMethodInvocation *context)
> @@ -2660,6 +2657,159 @@ ck_manager_get_sessions (CkManager  *manager,
>          return TRUE;
>  }
>
> +/*
> +  Example:
> +  dbus-send --system --dest=org.freedesktop.ConsoleKit \
> +  --type=method_call --print-reply --reply-timeout=2000 \
> +  /org/freedesktop/ConsoleKit/Manager \
> +  org.freedesktop.ConsoleKit.Manager.CreateSession \
> +  int32:101 string:"Headless" string:"/dev/local"
> +*/
> +gboolean
> +ck_manager_create_session (CkManager              *manager,
> +                           gint32                  display_number,
> +                           char                   *display_type,
> +                           char                   *tty_device,
> +                           char                  **id,
> +                           DBusGMethodInvocation  *context)
This API doesn't link a session to a seat.  We want to be able to
start a new session on an existing seat. So maybe there should be
a Manager.CreateSeat call, and then a Seat.CreateSession call.

[...]
> +static void
> +listify_created_session_displays (char       *id,
> +                                  CkSession  *session,
> +                                  char      **session_list)
> +{
> +        gboolean is_dynamic;
> +        char     *display;
> +        GError  *error;
error isn't used here.
> +
> +        ck_session_is_dynamic (session, &is_dynamic, NULL);
> +
> +        if (!is_dynamic)
> +                return;
> +
> +        ck_session_get_x11_display (session, &display, NULL);
> +        if (*session_list == NULL) {
> +                *session_list = g_strdup (display);
> +        } else {
> +                *session_list = g_strdup_printf ("%s;%s", *session_list, display);
> +        }
> +}
> +
> +/*
> +  Example:
> +  dbus-send --system --dest=org.freedesktop.ConsoleKit \
> +  --type=method_call --print-reply --reply-timeout=2000 \
> +  /org/freedesktop/ConsoleKit/Manager \
> +  org.freedesktop.ConsoleKit.Manager.ListCreatedSessions
> +*/
Does this API add anything over just calling GetSeats() and filtering
out those that are dynamic?

> diff --git a/src/ck-seat.c b/src/ck-seat.c
> index c2f70da..9dc1b9f 100644
> --- a/src/ck-seat.c
> +++ b/src/ck-seat.c
[...]
> +static void
> +_create_display (gpointer data,
> +                 gpointer user_data)
> +{
> +        CkSeat      *seat = CK_SEAT (user_data);
> +        DisplayData *disp = (DisplayData *)data;
> +        DBusGProxy  *proxy;
> +        gboolean     ret = TRUE;
> +        GError      *error = NULL;
> +
> +        proxy = dbus_g_proxy_new_for_name (seat->priv->connection,
> +                                           GDM_DBUS_NAME,
> +                                           GDM_DBUS_LOCAL_DISPLAY_FACTORY_PATH,
> +                                           GDM_DBUS_LOCAL_DISPLAY_FACTORY_INTERFACE);
This isn't allowed.  We can't assume ConsoleKit is running on a
machine with GDM.  ConsoleKit should probably emit a signal here
instead (or call the method on whatever display manager is managing
the seat via the ManageSeat() api mentioned above.

> +        if (proxy == NULL) {
> +                g_critical ("error getting %s proxy",
> +                            GDM_DBUS_LOCAL_DISPLAY_FACTORY_INTERFACE);
> +                return;
> +        }
> +
> +        error = NULL;
> +        g_free (disp->display_id);
> +        ret = dbus_g_proxy_call (proxy,
> +                                 "CreateDisplay",
> +                                 &error,
> +                                 G_TYPE_STRING, seat->priv->id,
> +                                 G_TYPE_INT, disp->num,
> +                                 G_TYPE_STRING, ck_display_type_get_command (seat->priv->display_type) ? ck_display_type_get_command (seat->priv->display_type) : "",
> +                                 G_TYPE_STRING, ck_display_type_get_arguments (seat->priv->display_type) ? ck_display_type_get_arguments (seat->priv->display_type) : "",
> +                                 G_TYPE_BOOLEAN, ck_display_type_get_chooser (seat->priv->display_type),
> +                                 G_TYPE_BOOLEAN, ck_display_type_get_use_auth (seat->priv->display_type),
> +                                 G_TYPE_INT, ck_display_type_get_priority (seat->priv->display_type),
> +                                 G_TYPE_STRING, disp->tty_device ? disp->tty_device : "",
> +                                 G_TYPE_BOOLEAN, disp->dynamic,
> +                                 G_TYPE_INVALID,
> +                                 DBUS_TYPE_G_OBJECT_PATH, &disp->display_id,
> +                                 G_TYPE_INVALID);

Maybe the call should be an a{sv} isntead of a long list of args.  I
mean if we need to send that mean arguments now, we might need to add
more later...
[...]
> +gboolean
> +ck_seat_add_display (CkSeat     *seat,
> +                     int         num,
> +                     gboolean    dynamic,
> +                     const char *tty)
> +{
> +        DisplayData *disp;
> +
> +        disp = g_new0 (DisplayData, 1);
> +
> +        disp->num = num;
> +        disp->dynamic = dynamic;
> +        disp->tty_device = g_strdup (tty);
> +
> +        seat->priv->displays = g_slist_append (seat->priv->displays, disp);
> +}
This should have a void return type.  Also you've got some extra whitespace on
the blank line.
> +
> +gboolean
> +ck_seat_set_display_type (CkSeat *seat,
> +                          char   *type)
> +{
> +        g_object_unref (seat->priv->display_type);
> +        seat->priv->display_type = ck_display_type_new (type);
> +}
This should have a void return type.

> +
> +gboolean
> +ck_seat_get_display_id (CkSeat  *seat,
> +                        int      num,
> +                        char   **id)
> +{
> +        int i, len;
> +        DisplayData *disp;
> +
> +        len = g_slist_length (seat->priv->displays);
> +
> +        for (i = 0; i < len; i++) {
> +
> +                disp = (DisplayData *) g_slist_nth_data (seat->priv->displays, i);
> +
> +                if (disp->num == num) {
> +                        g_free (*id);
> +                        *id = g_strdup (disp->display_id);
> +                        break;
> +                }
> +
> +        }
> +
> +}
This function doesn't return any value but it's return type is
boolean.  It should probably
return a string instead of use an out parameter, yea?
>  CkSeat *
>  ck_seat_new_from_file (const char *sid,
>                         const char *path)
>  {
[...]
> @@ -1107,6 +1299,45 @@ ck_seat_new_from_file (const char *sid,
>                  return NULL;
>          }
>
> +        error = NULL;
> +        str = g_key_file_get_string (key_file, group, "DisplayType", &error);
> +        if (error) {
> +                g_warning ("Unable to load key DisplayType in seat file: %s", path, error->message);
You're missing a %s here
[...]
> +        tty_device_list = g_key_file_get_integer_list (key_file, group, "TtyDevices", &ntty_device, NULL);
> +
>          device_list = g_key_file_get_string_list (key_file, group, "Devices", &ndevices, NULL);
>
>          g_debug ("Creating seat %s with %d devices", sid, ndevices);
ndevices should be cast to int here.

--Ray