Light Display Manager

Code review comment for lp:~rhansen/lightdm/multiseat

multiseat
Merge into trunk

Revision history for this message

Richard Hansen (rhansen) wrote on 2013-08-12:

Thanks for the review! I'll work on addressing your comments.

Do you have any comments about the prerequisite branches (lp:~a7x/lightdm/seatunity-call-parent and lp:~a7x/lightdm/multiseat-logging)?

> The xserver-vt option seems a bit odd - you don't want the
> configuration to define a particular VT,

Revision 1764 [1] added xserver-vt specifically to enable the user to manually override the VT that LightDM would otherwise choose. This was how I got multiseat running initially -- I set both seats to VT 7 and manually added the -sharevts command-line argument via the xserver-command setting. My lightdm.conf looked something like this:

    [Seat:0]
    xdg-seat=seat0
    xserver-vt=7

    [Seat:1]
    xdg-seat=seat1
    xserver-vt=7
    xserver-command=/usr/bin/X -sharevts

Later I added revision 1765 [2] to extended the semantics of the xserver-vt option so that 0 means -sharevts. This enabled me to get rid of the xserver-command line from seat 1 and the xserver-vt line from seat 0:

[Seat:0]
xdg-seat=seat0

    [Seat:1]
    xdg-seat=seat1
    xserver-vt=0

I used VT 0 for this purpose because Linux does not have a VT 0 [3]. I didn't think about other *nix OSes at the time; supposedly SysV had a VT 0, so other systems might too. So, in hindsight, hijacking VT 0 to mean -sharevts might not be the best idea. (It's also a bit odd from a user perspective as you mention.)

While I agree that the functionality provided by xserver-vt=0 should be provided via some other configuration mechanism (e.g., use-vt=false), I think there is still value in allowing the user to manually override the VT that LightDM would normally choose. But that's not really related to multiseat, so I can take it out of this branch.

[1] https://bazaar.launchpad.net/~a7x/lightdm/multiseat/revision/1764
[2] https://bazaar.launchpad.net/~a7x/lightdm/multiseat/revision/1765
[3] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/vt/vt_ioctl.c#n45

> but instead if LightDM should assign them.
>
> I think it would work better with:
>
> [SeatDefaults]
> use-vt=false
>
> Then seat-xlocal.c and seat-unity.c wouldn't call
> xserver_local_set_vt () if this was false.

Presumably use-vt=false would also:
* not pass vtXX to X
* pass -sharevts to X
Right?

I'm tempted to also support use-vt=<integer> in addition to true and false (if easily supported). This would allow the user to force LightDM to use VT <integer> rather than automatically picking one. Thoughts?

The behavior of GDM with systemd, and the recommended configuration I've seen (without any official documentation to back it up), is to do something like this:

* main seat: /usr/bin/X -seat seat0 vt7
* other seats: /usr/bin/X -seat seatX -sharevts

I don't fully understand the interaction between the kernel, Plymouth, and X when it comes to virtual terminals, but setting use-vt=false for all seats would not match the above behavior. I'm not sure what would happen if I did do -sharevts without vtXX on all seats.

I expect the right multiseat config would look like this:

[SeatDefaults]
use-vt=false

    [Seat:0]
    xdg-seat=seat0
    use-vt=true

[Seat:1]
xdg-seat=seat1

[Seat:2]
xdg-seat=seat2

# etc.

> The sample configuration comments are very wordy, i.e.
[snip]
>
> I'd do something more like:
>
> xdg-seat=<name>: Set the XDG name for this seat
>
> and lower down
>
> xdg-seat=seat0
>
> (implying that seat0 is the default)

I should note that unset xdg-seat and xdg-seat=seat0 are not equivalent. xdg-seat=seat0 causes '-seat seat0' to be passed to X, but unset xdg-seat does not.

From browsing the X source code, passing '-seat seat0' to X appears to be a no-op, but:
  * I'm not 100% certain
  * the X man page does not guarantee that '-seat seat0' is a no-op
  * I didn't want to introduce any behavior change from the current
    defaults (which is also why I don't derive the XDG_SEAT name from
    the title of the [Seat:foo] section in lightdm.conf)

The sample config file is not really the place for a detailed specification (there really should be a lightdm.conf(5) man page...) so I'll simplify the text.

Thanks for the review!  I'll work on addressing your comments.

Do you have any comments about the prerequisite branches (lp:~a7x/lightdm/seatunity-call-parent and lp:~a7x/lightdm/multiseat-logging)?

> The xserver-vt option seems a bit odd - you don't want the
> configuration to define a particular VT,

Revision 1764 [1] added xserver-vt specifically to enable the user to manually override the VT that LightDM would otherwise choose.  This was how I got multiseat running initially -- I set both seats to VT 7 and manually added the -sharevts command-line argument via the xserver-command setting.  My lightdm.conf looked something like this:

[Seat:0]
    xdg-seat=seat0
    xserver-vt=7
    
    [Seat:1]
    xdg-seat=seat1
    xserver-vt=7
    xserver-command=/usr/bin/X -sharevts

Later I added revision 1765 [2] to extended the semantics of the xserver-vt option so that 0 means -sharevts.  This enabled me to get rid of the xserver-command line from seat 1 and the xserver-vt line from seat 0:

[Seat:0]
    xdg-seat=seat0
    
    [Seat:1]
    xdg-seat=seat1
    xserver-vt=0

I used VT 0 for this purpose because Linux does not have a VT 0 [3].  I didn't think about other *nix OSes at the time; supposedly SysV had a VT 0, so other systems might too.  So, in hindsight, hijacking VT 0 to mean -sharevts might not be the best idea.  (It's also a bit odd from a user perspective as you mention.)

While I agree that the functionality provided by xserver-vt=0 should be provided via some other configuration mechanism (e.g., use-vt=false), I think there is still value in allowing the user to manually override the VT that LightDM would normally choose.  But that's not really related to multiseat, so I can take it out of this branch.

> but instead if LightDM should assign them.
>
> I think it would work better with:
>
>     [SeatDefaults]
>     use-vt=false
>
> Then seat-xlocal.c and seat-unity.c wouldn't call
> xserver_local_set_vt () if this was false.

Presumably use-vt=false would also:
  * not pass vtXX to X
  * pass -sharevts to X
Right?

I'm tempted to also support use-vt=<integer> in addition to true and false (if easily supported).  This would allow the user to force LightDM to use VT <integer> rather than automatically picking one.  Thoughts?

The behavior of GDM with systemd, and the recommended configuration I've seen (without any official documentation to back it up), is to do something like this:

* main seat: /usr/bin/X -seat seat0 vt7
  * other seats: /usr/bin/X -seat seatX -sharevts

I don't fully understand the interaction between the kernel, Plymouth, and X when it comes to virtual terminals, but setting use-vt=false for all seats would not match the above behavior.  I'm not sure what would happen if I did do -sharevts without vtXX on all seats.

I expect the right multiseat config would look like this:

[SeatDefaults]
    use-vt=false
    
    [Seat:0]
    xdg-seat=seat0
    use-vt=true
    
    [Seat:1]
    xdg-seat=seat1
    
    [Seat:2]
    xdg-seat=seat2
    
    # etc.

I should note that unset xdg-seat and xdg-seat=seat0 are not equivalent.  xdg-seat=seat0 causes '-seat seat0' to be passed to X, but unset xdg-seat does not.

The sample config file is not really the place for a detailed specification (there really should be a lightdm.conf(5) man page...) so I'll simplify the text.

« Back to merge proposal