Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1351
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1351>
> >
> >     you can leave out the "automatic multiseat won't be enabled" from the followup messages.
> >     
> >     isn't there a function to turn the error code into a string?
> 
> Stefan Brüns wrote:
>     Regarding error codes, the calls are documented as:
>     "On failure, these calls return a negative errno-style error code."

ah, then you can just assign it to errno and use %m in the format string. there is a bunch of examples for that.


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1397
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186612#file186612line1397>
> >
> >     that seems questionable to me. why are you re-defining the display to be permanent? when the seat goes away, kdm won't know what to do with it.
> 
> Stefan Brüns wrote:
>     When a seat goes away, rStopDisplay(d, DS_RESERVE) is called - maybe a "d->displayType = dLocal | dReserve" is missing for these cases.
>     But as long as the seat exists, you want it to behave like a static display (restart server after session exit), so dPermanent is IMHO correct.

i see what you mean ...
still, it seems wrong to change the display type just so. this is supposed to be a constant. this is of course a consequence of abusing reserve displays in the first place - technically, there should be a dDynamic display type. if having reserve displays on dynamic seats is technically possible at all, this would also need some thought.


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/server.c, line 79
> > <https://git.reviewboard.kde.org/r/112294/diff/2/?file=186613#file186613line79>
> >
> >     why the redundant supply of the seat as a layout?
> 
> Stefan Brüns wrote:
>     There are no config matches taking the "seat" into account, so the only possibility to apply a specific config is to use "layout".

something is wrong about this.
what does the -seat option do in the first place? if it is sufficient to actually launch the server correctly, then that's all that should be done - the user can manually specifiy ServerArgs if he needs to configure the layout for some reason. otherwise every user is forced to actually have a "seatN" layout in his config.
also, cannot the layout determine the seat? seems a bit stupid that these are entirely orthogonal.


- Oswald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/112294/#review54418
-----------------------------------------------------------


On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -----------------------------------------------------------
> 
> (Updated March 29, 2014, 5:38 p.m.)
> 
> 
> Review request for kde-workspace and Oswald Buddenhagen.
> 
> 
> Repository: kde-workspace
> 
> 
> Description
> -------
> 
> This patch implements dynamic multiseat in KDM. It follows the description in:
> http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/
> 
> In case systemd is no found at compile time, nothing changes. If logind is not running, nothing changes. If no additional seats have been configured (some Plugable USB-GPUs are automatically added as additional seats), nothing changes.
> 
> In case there are additional seats beyond seat0, a reserved display is promoted to a local static one (and demoted if the seat is removed) and a new X-Server/greeter is spawned.
> 
> The code has been tested extensively, with a combination of [Radeon dedicated GPU|Intel iGPU], [Intel iGPU|Displaylink USB GPU] and others. For history of this patch, see https://bugzilla.redhat.com/show_bug.cgi?id=884271 and https://bugzilla.redhat.com/show_bug.cgi?id=975079
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt d5c76d8 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   cmake/modules/FindSystemd.cmake PRE-CREATION 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c 26bb0b4 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
> 
> Diff: https://git.reviewboard.kde.org/r/112294/diff/
> 
> 
> Testing
> -------
> 
> Single seat system, several multiseat systems
> 
> 
> Thanks,
> 
> Stefan Brüns
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140330/284a22e4/attachment.htm>