----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/112294/#review58394 -----------------------------------------------------------
i didn't finish the review, because it seems that revision 6 is itself an interdiff to a previous attempt. maybe you forgot to squash? please factor out the fixes into separate commits, for atomicity. regarding debug statements in production code: it's ok to leave some in. but look at them from the admin perspective: does its presence help to debug config/runtime problems, or is it really only for the developer? the latter ones should be eliminated in the end. kdm/backend/dm.h <https://git.reviewboard.kde.org/r/112294/#comment40621> i'd remove the Reserve "suffix". it just adds verbosity. kdm/backend/dm.h <https://git.reviewboard.kde.org/r/112294/#comment40622> this is an enum, not a bit field. you can make the value 6, then you don't need to change anything else. kdm/config.def <https://git.reviewboard.kde.org/r/112294/#comment40620> this should come before the ReserveServers - as mentioned before, hypothetically, you could have reserve displays for dynamic servers, so they are "least significant". kdm/config.def <https://git.reviewboard.kde.org/r/112294/#comment40615> 10 is an unwise offset, as it clashes with the ssh default. that's why i kept it below that in all my examples. i don't think there will be more than 10 local displays in any realistic scenario (the machine would explode anyway). regarding DynamicDisplays and your magic backwards compat handling ... i think the seat-encoded config stuff i insisted on makes that redundant, after all. suppose we do it like that: StaticServers=:0,@seat1_:4,@seat2_:7 ReserveServers=:1,:2,@seat1_:5,@seat2_:8 local displays without a seat are implicitly on seat0. that's your special case, without much magic. if kdm is started with systemd, *all* static local servers actually go straight into the dynamic state and need to be activated by systemd. if kdm is compiled with systemd support but none is running, displays on seats != 0 are simply eliminated (with a notice to the log). if kdm is compiled without systemd support, seats != 0 are rejected (with an error to the log). if somebody feels like implementing actual support for that, they can. as before, the ReserveServers are only meant for demo purposes. don't worry about them. would that work? - Oswald Buddenhagen On May 18, 2014, 7:44 p.m., Stefan Brüns wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/112294/ > ----------------------------------------------------------- > > (Updated May 18, 2014, 7:44 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 > ----- > > kdm/backend/dm.h b2f8c61 > kdm/backend/dm.c 77a2ef7 > kdm/backend/dpylist.c b650c2f > kdm/backend/resource.c 38a8c70 > kdm/config.def 751c0ed > kdm/kfrontend/kdm_config.c 368c8d1 > > Diff: https://git.reviewboard.kde.org/r/112294/diff/ > > > Testing > ------- > > Single seat system, several multiseat systems > > > Thanks, > > Stefan Brüns > >
