Re: [gdm-list] Patches for multi-seat and multi-display support
- From: Halton Huo <Halton Huo Sun COM>
- To: Ray Strode <halfline gmail com>
- Cc: gdm-list gnome org
- Subject: Re: [gdm-list] Patches for multi-seat and multi-display support
- Date: Thu, 14 May 2009 16:14:03 +0800
> Can you figure out X11_DIR from pkg-config?
Done, thanks for Dan's patch.
> 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.
Done.
>
> > 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.
Done.
> > +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)
Separator ';' and ',' are from GKeyFile. I did not find any API to only
allow ';'. Can I simply change the comment that only allow ';'? That is
to say, even ',', the list can be recognized by GKeyFile.
>
> 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.
After reading the following comments, most of them is about how to
represent seat and display.
I have to say the design
http://wiki.genunix.org/wiki/index.php/design_for_newgdm_consolekit_multiseat_multidisplay is not get well reviewed before coding.
I'd suggest we make a agreement on the configuration files first, then I can go with the right way.
I'd like summarize what your said and work out the updated design for
review, does it sound reasonable?
> > +
> > +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.
Same with above.
> 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.
Same with above.
>
> > +
> > +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.
Same with above.
>
> > 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.
>
Same with above.
> > +
> > +[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
>
Same with above.
> > --- /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?"
As Brian said, this feature is dropped for now.
>
> [...]
> > +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.
Will be removed since we decide to remove priority feature now.
>
> [...]
> > +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 ?
Will be removed since we decide to remove priority feature now.
> [...]
> > +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.
Reasonable, will do.
>
> > + 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.
Will do after the new design is ready.
>
> [...]
> > 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...
> [...]
Good catch, removed.
> > +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.
Good suggestion, this need time to change, will do.
> > 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.
This api is supposed to used by "ck-dynamic -a -t <type> -n <disp_num>" to create a dynamic display.
Normally each call of "ck-dynamic -a" will create a new seat with a new session attached on it.
If ck-dynamic is designed as can adding a session for existing seat, then this API should be changed.
So the question is do we need that feature for ck-dynamic?
>
> [...]
> > +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?
Agree with you and Brian, will remove this API from CkManager, and let ck-dynamic do that itself.
>
> > 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.
Agree, will change.
>
> > + 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...
can not agree more, will do.
> [...]
> > +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.
To keep function same style, I'd like keep this function return gboolean, and adding following code
g_return_val_if_fail (CK_IS_SEAT (seat), FALSE);
[...]
return TRUE;
> > +
> > +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.
Same with above.
>
> > +
> > +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?
Same with above.
> > 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
Good catch, done.
> [...]
> > + 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.
Done.
Totally, I have 6 items to do together with my estimation:
1) redesign on how to represent seat and display (5h)
2) removing listify_created_session_displays (0.5h)
3) rework ck_manager_create_static_sessions (3h)
4) use a{sv} for dbus method CreateDisplay (2h)
5) rewrite ck_display_type_new (1h)
6) remove priority code (1h)
Will keep you updated.
Ray, can I commit to those two branches on changes that we agree?
My gnome account is "haltonhuo".
For freedesktop, I just followed the page http://freedesktop.org/wiki/AccountRequests
to require an account, https://bugs.freedesktop.org/show_bug.cgi?id=21732.
Could you or Jon help my request?
Thanks,
Halton.
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
