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

[Halton Huo]

On Mon, 2009-06-29 at 18:53 -0400, Ray Strode wrote:
> 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.
Great to hear your feedback, please see my comments in lines.

> > +# 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.
I saw your change on your private repo, that seems fine to me.

> > +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?
(I agree to with your idea use suffix .display rather than .disp)
At least Local.display should be shipped to allow user start a local
session without any change. Others can be removed from repo. Is that
okay for you?

> > +enum {
> > +        PROP_0,
> > +        PROP_NAME,
> > +        PROP_COMMAND,
> > +        PROP_PRIORITY,
> You still have PRIORITY here.
Good catch, safe to remove that line.

> > +        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 :-)
As I said above, it is okay for me to use .display.

> 
> > 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.
True. Actually, I'd like to ask what the word "manage" and "unmanaged"
mean. I ever thought an unmanaged seat is a seat has a session attach on it.
Am I correct?

> > +/*
> > +  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.
This related your idea on redesign ck-dynamic, check below.

> 
> > +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.
Good idea! I can add option "-s, --seat" to pass the seat_id in.

> > +/*
> > +  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.
This related your idea on redesign ck-dynamic, check below.

> 
> > +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)
Then, I should add a function ck_seat_remove_session for CkSeat which emit this signal. And
ck_manager_remove_session() call ck_seat_remove_session() instead. Okay for you?

> 
> > --- 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
Good catch.

> [...]
> > +#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
Agree!

> > +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.
No, variable seat is used. Please check the code.

_create_display (gpointer data,
                 gpointer user_data)
{
        CkSeat      *seat = CK_SEAT (user_data);
[...]
        comm = g_strdup (ck_display_type_get_command
(seat->priv->display_type));

[...]
        g_signal_emit (seat, signals [SESSION_TO_ADD], 0, disp->dynamic,
comm);

        g_free (comm);

}                    

> 
> > +
> > +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.
Okay for me.

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

> 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.
Saw your changes on your repo, good to me.

What Type=X11 in .session for?
> 
> 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 do not why do this. Is the session id requested for OpenSessionWithParameters?

> 
> I also think it would make sense to rename the signal to SessionToOpen or
> OpenSessionRequest instead of SessionToAdd (likewise for SessionToRemove).
Okay for me of the name changing.

> 
> 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
Great! I can understand your idea better by referring your code.

> 
> 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
I presume the "port=xxx" and password="yyy" is extra arguments need to append on
Exec in .display. Correct?

--num is needed for ck-dynamic. Sunray need create hundreds of display, nomally start from :10.

Without "ck-dynamic --num", administrators need generate hundreds of static.session files,
that is impossible.

To let ck-dynamic more powerful, I'd like redesign ck-dynamic to satisfy your request.

ck-dynamic -a
=============

   $ck-dynamic -a --session-type=<session_type> --display-type=<display_type> [--seat-id=<seat_id>]
OR
   $ck-dynamic -a --num=<display_number> [--tty=<display_type>] --display-type=<display_type> [--seat-id=<seat_id>]
OR
   $ck-dynamic -a --session-type=<session_type> --command=<command> [--seat-id=<seat_id>]
OR
   $ck-dynamic -a --num=<display_number> [--tty=<display_type>] --command=<command> [--seat-id=<seat_id>]

--num and --tty represent a virtual .session file.

--command represent a virtual .display file.


ck-dynamic -d
=============
   $ck-dynamic -d --num=<display_number> (Needed for SunRay)
OR
   $ck-dynamic -d --session-id=<sessoin_id>

ck-dynamic -l
=============
   $ck-dynamic -l
   List all attached displays. The result is separated by ";", for example, ":110;:111"

Examples:
============
1. For your above case
   $ck-dynamic -a --session-type=LoginWindow --command="Xvnc $display -auth $auth -query localhost port=5908 password-file=/etc/X11/vnc-password"
OR
   $ck-dynamic -a --num=64 --command="Xvnc $display -auth $auth -query localhost port=5908 password-file=/etc/X11/vnc-password"

2. Adding a :1 on vt8 for existing seat1
   $ck-dynamic -a --num=1 --tty=8 --display-type=Local --seat-id=Seat1

Any comments?
> 
> - ck-dynamic probably doesn't have the right name. ck-seat-tool ? not sure
>   what a better name would be.
Okay for me for the name changing.

> 
> Anyway, we're making really good progress.
Glad to know you're happy.

I saw your repo is under working. If you can list some tasks based on
your repo, I'm happy to take some you assign.

Cheers,
Halton.