GNOME.org
 

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


Ray:

I'm sure Halton will have more comments.  A few items I wanted to
discuss a bit.


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.


Should we use a keyword instead of a number to indicate "I don't care"?


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


Since "-nolisten tcp" is added via GDM configuration, it might not
make sense to add it in the configuration file unless the user really
wants to override the configuration setting, for some reason, and always
add the argument.


+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.


I think this could work either way.  I think the difficulty here is
that some arguments get added on-the-fly, such as "-nolisten tcp'
and the $display and $vt values.

Specifying the arguments allows GDM to do its magic adding arguments
that are needed, and allows the user to specify additional arguments
that might be needed.  Perhaps a flag to enable a needed extension or
something.

With your suggestion above, how would GDM behave if you left out the
"$vt" and the user obviously intends to start a VT display?  Should
GDM add the argument anyway, or should it fail because you messed up
the config file?

The advantage of specifying additional arguments is that you do not
end up in situations where the command is specified incorrectly.


+
+[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.,


In our first proposal, we suggested having these in separate files.
But, in private discussion on March 4, 2009, you said the following:

> Well, let me take a step back and say somethings that I think are
> right:
>
> 1) A seat represents the monitor, keyboard, mice, and usb hub that the > user is sitting at. 
> 2) Usually there is one display (X server) per seat (although things
>    get a little muddied when you think about fast user switching)
> 3) There is one session at a time per display
> 4) There are only a handful of display types (local X server, XDMCP X
>    server, Xvfb X server for SunRay)
> 5) There could be a large number of seats (and correspondingly a large
> number of displays)
>
> Given 4 and 5, we should have two sets of definitions (not sure what
> Jon's opinion is here).  One that defines the display types (basically
> what canned X server command lines to run), and one that defines the
> static seat configuration that uses those display types (basically X
> servers to start at start up).
>
> So I could imagine having some displays types (forgive the contrived
> syntax):
>
> {Local: command=/usr/bin/Xorg $display -br -verbose -auth $auth
>  -nolisten tcp $vt},
> {Remote Machine: command=/usr/bin/Xorg $display -br -verbose -auth
> $auth -indirect $vt}
> {Local VNC: command=/usr/bin/Xvnc $display -auth $auth -query
> localhost}
> {Headless: command=/usr/bin/Xvfb $display -auth $auth}
>
> and then a static configuration like:
>
> {Seat 1: type=Local}, {Seat 2: Remote Machine, vt=12}, {Seat 3: Local
> VNC}, {Seat 4: Headless}, {Seat 5: Headless}, {Seat 6: Headless}
>
> Which would start a normal X server on the first available vt, a
> special X server that brings up a chooser on vt12, a local VNC server
> that allows remote clients to vnc in and login, and then 3 framebuffer
> displays.
>
> You could think of the display type as a "class" and the static
> configuration as "instances of the class".
>
> There could be other seats, too, that aren't statically configured,
> but dynamically set up at runtime.  For instance, GDM sets up some
> XDMCP displays.  Each time a user connects to GDM via XDMCP, a new
> remote seat would get created dynamically to logically map to that
> user's monitor, keyboard, and mouse.
>
> Now the question is how we represent that in config files.  I'd say
> you could do the static configuration by dropping files in
>
> /etc/ConsoleKit/seats.d
>
> but then you still need a place to define display types.  Maybe,
>
> /etc/ConsoleKit/displays.d

To me, it sounded like you were suggesting that the seats be defined in
a single static configuration file.  So we went with this approach.  I
don't think it's a problem to change this again, but I want to make sure
that we consider our previous logic before we change things again.


[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?"


The priority stuff was added, I think, because the old GDM supported
this.  I think some users find it handy to be able to specify what
priority the Xserver should be started with.

However, I suggest that we drop this logic from this patch and address
this later if it turns out to be something people really want.


+        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.


That seems a good idea.


+
+        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?


I'm now thinking that it probably doesn't make sense to provide an API
for doing this.  Client programs which want this feature can write the
code themselves, fairly simply.

Brian
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]