[ConsoleKit] [gdm-list] Reworked patch for multi-seat and multi-display support
Ray Strode
halfline at gmail.com
Mon Jun 29 15:53:36 PDT 2009
Hey Halton,
> Thanks for your comments at
> http://mail.gnome.org/archives/gdm-list/2009-May/msg00007.html
>
> I've finished the tasks below and now the new patch is ready for review.
I haven't forgotten about this! I've been working on the patches for
the last couple of weeks. I don't think they're ready yet, but I
wanted to give an update, since I've been sort of working in a bubble.
> 1 Use separate .disp file for display type, remove display-types.conf
> install .disp files to /etc/ConsoleKit/displays.d/,
> each .disp file represent a display type.
> 2 Only use Command in .disp file, remove Arguments/UseAuth/Chooser/Priority
> 3 Remove priority code
> 4 When adding/removing displays, CK should emit signal instead of
> calling GDM method directly
> For org.freedesktop.ConsoleKit.Seat
> add signal SessionToAdd and SessionToRemove
> add method ManageSeat
> For org.freedesktop.ConsoleKit.Manager
> remove method CreateStaticSessions
> add method GetUnmanagedSeats
> 5 Use a{sv} for dbus method CreateDisplay, since
> task #2 is done, there are only two parameters need to be send with
> signal, this work is not necessary
> 6 Rewrite ck-dynamic to reflect change in task #4
> 7 Rewrite ck_display_type_new()
> 8 Remove method org.freedesktop.ConsoleKit.Manager.ListCreatedSessions.
> Let ck-dynamic do the GetSeats() and filtering dynamic displays itself
> 9 make ck_seat_add_display(), ck_seat_set_display_type() and ck_seat_get_displ
> return a gbooolean
> 10 Remove unused create_display_with_parameters()
> 11 Comments should appear before the lines they're commenting on, not after.
> 12 Reverse Name and Version back
> 13 Figure out X11_DIR from pkg-config
All of these sound good.
> +# Display numbers list, separated by a separator character ';'. If want to
> +# automatically choose next available display number, indicate -1
> +DisplayNumbers=0
> +
> +# Values list to indicate whether the give DisplayNumbers are on console
> +# or not , separated by a separator character ';'.
> +IsConsoles=true
> +
> +# Values list to indicate the tty device number, separated by a separator
> +# character ';'.
> +# Zero or negative number or no value means choose next available vt number
> +TtyDevices=
So I think the best plan here is to make another config file that
bridges displays to seats.
Looking at the ConsoleKit code there's already that concept
internally. It's called a "session",
so having a separate set of session config files seems to be better
than parallel keys like this.
> diff --git a/data/displays.d/Headless.disp.in b/data/displays.d/Headless.disp.in
> new file mode 100644
> diff --git a/data/displays.d/Local.disp.in b/data/displays.d/Local.disp.in
> new file mode 100644
> diff --git a/data/displays.d/LocalVNC.disp.in b/data/displays.d/LocalVNC.disp.in
> new file mode 100644
> diff --git a/data/displays.d/Makefile.am b/data/displays.d/Makefile.am
> new file mode 100644
> index 0000000..bb305b2
> diff --git a/data/displays.d/RemoteMachine.disp.in b/data/displays.d/RemoteMachine.disp.in
[...]
> +displaydir = $(sysconfdir)/ConsoleKit/displays.d
> +display_in_files = \
> + Local.disp.in \
> + RemoteMachine.disp.in \
> + LocalVNC.disp.in \
> + Headless.disp.in
> +
> +display_DATA = $(display_in_files:.disp.in=.disp)
Hmm, these are really just examples though. Maybe they should be
noinst? or only shipped in the docs?
> diff --git a/src/ck-display-type.c b/src/ck-display-type.c
> @@ -0,0 +1,240 @@
> +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*-
> + *
> + * Authors: halton.huo at sun.com
> + * Copyright (C) 2009 Sun Microsystems, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + *
> + */
> +
> +#include "config.h"
> +
> +#include <glib.h>
> +#include <glib-object.h>
> +
> +#include "ck-display-type.h"
> +
> +#define CK_DISPLAY_TYPE_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), CK_TYPE_DISPLAY_TYPE, CkDisplayTypePrivate))
> +
> +#define CK_DISPLAY_TYPES_DIR SYSCONFDIR "/ConsoleKit/displays.d"
> +
> +struct CkDisplayTypePrivate
> +{
> + char *name;
> + char *command;
> +};
> +
> +enum {
> + PROP_0,
> + PROP_NAME,
> + PROP_COMMAND,
> + PROP_PRIORITY,
You still have PRIORITY here.
...
> +static void
> +ck_display_type_load (CkDisplayType *display)
> +{
> + GKeyFile *key_file;
> + const char *name;
> + char *filename;
> + char *command;
> + gboolean res;
> + GError *error;
> +
> + name = ck_display_type_get_name (display);
> +
> + g_return_if_fail (name && !g_str_equal (name, ""));
> +
> + filename = g_strdup_printf ("%s/%s.disp", CK_DISPLAY_TYPES_DIR, name);
Any particular reason you're abbreviating 'display' in the file
extension? It only saves 3 letters :-)
> diff --git a/src/ck-manager.c b/src/ck-manager.c
> index bcb9350..1119e95 100644
> --- a/src/ck-manager.c
> +++ b/src/ck-manager.c
> +/*
> + Example:
> + dbus-send --system --dest=org.freedesktop.ConsoleKit \
> + --type=method_call --print-reply --reply-timeout=2000 \
> + /org/freedesktop/ConsoleKit/Manager \
> + org.freedesktop.ConsoleKit.Manager.GetUnmanagedSeats
> +*/
> +gboolean
> +ck_manager_get_unmanaged_seats (CkManager *manager,
> + GPtrArray **seats,
> + GError **error)
> +{
> + g_return_val_if_fail (CK_IS_MANAGER (manager), FALSE);
> +
> + if (seats == NULL) {
> + return FALSE;
> + }
> +
> + *seats = g_ptr_array_new ();
> + g_hash_table_foreach (manager->priv->seats, (GHFunc)listify_seat_ids, seats);
> +
> + return TRUE;
> +}
> +
This function seems to return both managed and unmanaged seats.
> +/*
> + 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"
> +*/
I don't really like that this function takes a display number.
Not all console kit sessions are X11 sessions after all.
If we're going to have a CreateSession function I think it should
be generic enough to accomodate all the types of sessions consolekit
supports.
> +gboolean
> +ck_manager_create_session (CkManager *manager,
> + gint32 display_number,
> + char *display_type,
> + char *tty_device,
> + DBusGMethodInvocation *context)
> +{
> + CkSeat *seat;
> + CkSession *session = NULL;
> +
> + g_return_val_if_fail (CK_IS_MANAGER (manager), FALSE);
> +
> + /* Make sure this display_number is not avail */
> + session = CK_SESSION (g_hash_table_find (manager->priv->sessions,
> + _find_session_with_num,
> + &display_number));
> +
> + if (CK_IS_SESSION (session)) {
> + return TRUE;
> + }
> +
> + seat = add_new_seat (manager, CK_SEAT_KIND_DYNAMIC);
The other thing I don't like about this function is that it unconditionally
creates a new seat. For fast user switching it would be useful to be able to
create a session on an existing seat.
I propose splitting this up into two parts: one function to create a new seat,
and another function to create a new session on the seat.
> +/*
> + Example:
> + dbus-send --system --dest=org.freedesktop.ConsoleKit \
> + --type=method_call --print-reply --reply-timeout=2000 \
> + /org/freedesktop/ConsoleKit/Manager \
> + org.freedesktop.ConsoleKit.Manager.RemoveSession \
> + int32:101
> +*/
Just like with CreateSession, I don't like how RemoveSession works
in terms of display numbers. Seems like it would be better to remove
a session via it's session id, instead of it's display number.
> +gboolean
> +ck_manager_remove_session (CkManager *manager,
> + gint32 display_number,
> + DBusGMethodInvocation *context)
> +{
[...]
> +
> + g_signal_emit_by_name (seat, "session-to-remove", display_number);
Emitting signals from another object like this is bad form (unless they're
action signals)
> --- a/src/ck-seat.c
> +++ b/src/ck-seat.c
> @@ -40,6 +40,7 @@
[...]
> #define NONULL_STRING(x) ((x) != NULL ? (x) : "")
> +#define N_ELEMENTS(arr) (sizeof (arr) / sizeof ((arr)[0]))
glib already has this macro defined as G_N_ELEMENTS
[...]
> +#ifndef HAVE_STRREP
> +static void
> +strrep (char* in, char** out, char* old, char* new)
> +{
[...]
> +}
> +#endif
This function could be replaced with g_regex_replace_eval
> +
> +static void
> +_create_display (gpointer data,
> + gpointer user_data)
> +{
> + CkSeat *seat = CK_SEAT (user_data);
> + DisplayData *disp = (DisplayData *)data;
> + char *comm = NULL;
> + char *str = NULL;
> +
> + comm = g_strdup (ck_display_type_get_command (seat->priv->display_type));
> +
> + /* subtitute $display */
> + if (disp->num >= 0) {
> + str = g_strdup_printf (":%d", disp->num);
> + strrep (comm, &comm, "$display", str);
> + g_free (str);
> + }
> +
> + /* subtitute $vt */
> + if (IS_STR_SET (disp->tty_device)) {
> + str = g_strdup_printf ("%s", disp->tty_device);
> + strrep (comm, &comm, "$vt", disp->tty_device);
> + g_free (str);
> + } else {
> + strrep (comm, &comm, "$vt", "");
> + }
> +
> + g_signal_emit (seat, signals [SESSION_TO_ADD], 0, disp->dynamic, comm);
> +
> + g_free (comm);
> +
> +}
> +
> +gboolean
> +ck_seat_manage_seat (CkSeat *seat,
> + DBusGMethodInvocation *context)
> +{
> + g_slist_foreach (seat->priv->displays, _create_display, seat);
> + return TRUE;
> +}
You're passing a method invocation argument here but never using it. That's
wrong. Either it should be a GError, or you need to dbus_g_method_return at
some point (which you do depends on what you have in the dbus interface
description for the method)
Also, the "Seat" in ManageSeat is redundant since it's a method on the
seat object.
> +
> +gboolean
> +ck_seat_add_display (CkSeat *seat,
> + int num,
> + gboolean dynamic,
> + const char *tty)
> +{
> + g_return_val_if_fail (CK_IS_SEAT (seat), FALSE);
> +
> + 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);
> +
> + return TRUE;
> +}
I don't think seats should deal with displays directly. As I mentioned earlier,
ConsoleKit already has the concept of "sessions" which belong to seats and
have displays associated with them. See the output of ck-list-sessions to
get a feel for the relationship. This should probably be ck_seat_add_session
not ck_seat_add_display.
> +
> +gboolean
> +ck_seat_set_display_type (CkSeat *seat,
> + char *type)
> +{
> + g_return_val_if_fail (CK_IS_SEAT (seat), FALSE);
> +
> + g_object_unref (seat->priv->display_type);
> + seat->priv->display_type = ck_display_type_new (type);
> +
> + return TRUE;
> +}
Again, it doesn't make sense for a seat to have only one display type,
since a seat can have multiple displays.
[...]
> CkSeat *
> ck_seat_new_from_file (const char *sid,
> const char *path)
> {
> @@ -1107,9 +1287,48 @@ ck_seat_new_from_file (const char *sid,
> return NULL;
> }
>
> + error = NULL;
> + str = g_key_file_get_string (key_file, group, "DisplayType", &error);
[...]
> + display_type = ck_display_type_new (str);
[...]
> + disp_num_list = g_key_file_get_integer_list (key_file, group, "DisplayNumbers", &ndisp_num, &error);
[...]
> + is_console_list = g_key_file_get_boolean_list (key_file, group, "IsConsoles", &nis_console, &error);
[...]
> + tty_device_list = g_key_file_get_integer_list (key_file, group, "TtyDevices", &ntty_device, NULL);
> +
[...]
> return seat;
> }
I propose instead of reading the DisplayType and those parallel keys here
we read a single "Sessions" list which is a list of session names
that are stored in /etc/ConsoleKit/sessions.d
Each session would have a display type associated with it along with data
about which display number and vt to use (or port for vnc or whatever).
In this set up the display type is actually a template and the session is an
instantiation of that template.
The CreateSession (or AddSession maybe?) would be a dynamic version of this.
Your patches are definitely going in the right general direction. They're
still missing some bits though.
For one, there needs to be some way for the display manager to acknowledge
that it's managing a particular static configuration. ConsoleKit needs to be
able to match up which sessions the display manager opens with which
sessions consolekit is asking for.
Note the display manager already calls OpenSessionWithParameters in response to
SessionToAdd. One of the parameters it passes could be the session id of the
session getting opened. This way ConsoleKit knows that the display manager
opened what it was asked to open, and can rerequest it gets opened after it's
closed.
I also think it would make sense to rename the signal to SessionToOpen or
OpenSessionRequest instead of SessionToAdd (likewise for SessionToRemove).
Another thing is display number and vt aren't the only bits of information a
particular display type might want filled out. For instance, you could
imagine an Xvnc server wanting a port number and password file. I think we
want to be able to specify those kinds of setups in the static seat
configuration as well. We need to be more generic.
I've made a start at trying to fix some of the above mentioned issues.
I've put that up on a branch here:
http://cgit.freedesktop.org/~halfline/ConsoleKit/log/?h=multi-seat
Some other random thoughts...
- I could see ConsoleKit potentially asking for getty sessions to get
opened as well (replacing /etc/inittab or upstart configuration) down the line.
- I feel like ck-dynamic is a little too limited. It would be really neat if we
could do
ck-dynamic -a --session-type "LoginWindow" --display-type "VNC" port=5908
password-file=/etc/X11/vnc-password
or something similar and have the display manager instantly,
dynamically create a vnc session with a login window on it.
- ck-dynamic probably doesn't have the right name. ck-seat-tool ? not sure
what a better name would be.
Anyway, we're making really good progress.
--Ray
(I'll be sending a mail to address your gdm patch to gdm-list shortly)
More information about the ConsoleKit
mailing list