Review Request 112294: Implement multi-seat support in KDM

Mon May 26 09:32:52 BST 2014

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

kdm/backend/client.c
<https://git.reviewboard.kde.org/r/112294/#comment40632>

    you forgot the free(envbuf).

    i don't like the interminglig with the other type of setting env vars.
    also, it's missing the pam ifdef.
    how about putting it right in front of line 1515?

    of course that means that hypothetically you also need to cover the !PAM case, which you would do exactly at this place.
    i'm not sure it makes sense to bother, tough - i don't suppose a system with systemd but without pam makes much sense?

kdm/backend/dm.h
<https://git.reviewboard.kde.org/r/112294/#comment40633>

    the comment is wrong.

    but ... i don't think you actually use that type anywhere to start with, which kind of makes sense.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40656>

    any particular reason why these are not static?

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40634>

    add spaces around binary operators.

    but ... you actually don't need the conditional at all: my printf implementation guarantees to print "(null)" for that.

    repeats several times.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40635>

    the inner parens are unnecessary, in particular the first ones.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40636>

    declaration after code ... nope, this is supposed to be c89-compatible.

    i prefer "ret" as the name for variables of this kind.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40637>

    instead of resetting it here, it may be better to assign the fd to the temp var first, and assign it "properly" below. it's likely that the compiler will make better code for that, too.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40638>

    why strchr? that's supposed to be always at [0] i would think ...

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40639>

    i strongly prefer at + 1, as otherwise it suggests just a particular array element, which is really not meant.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40642>

    i'm the guy who prefers a nice and clean goto over setting a variable which works around having a goto. you can jump straight to line 1439 here.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40640>

    "no matching unused"

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40641>

    if you intend to keep this statment: add space after the colon, and assign the value to a temporary, so the function is not called twice (unless it's inline/a macro, then don't bother).

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40643>

    also a case for goto.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40644>

    that would be a case for a direct return.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40648>

    that would be "ret" again.

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40647>

    preferably, put that into the if - ++failures >= ...

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40646>

    stringify(SYSTEMD_FAILURE_LIMIT) " failed..."

kdm/backend/dm.c
<https://git.reviewboard.kde.org/r/112294/#comment40645>

    i think that's pointless. the next one who uses the variable will re-init it itself.

kdm/backend/resource.c
<https://git.reviewboard.kde.org/r/112294/#comment40650>

    underscores are not permitted in dns names, so the colon thing should be unnecessary.

kdm/backend/resource.c
<https://git.reviewboard.kde.org/r/112294/#comment40649>

    cls + 1

kdm/backend/session.c
<https://git.reviewboard.kde.org/r/112294/#comment40655>

    what is the point of that? i don't think the greeter can use this for anything as-is.

the docu of StaticServers needs to be extended now.

i'm not sure the review is complete. need to run for work now. ^^

- Oswald Buddenhagen

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -----------------------------------------------------------
> 
> (Updated May 26, 2014, 12:06 a.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
> -----
> 
>   cmake/modules/CMakeLists.txt 117b3a5 
>   kdm/ConfigureChecks.cmake b61fd90 
>   kdm/backend/CMakeLists.txt 25f383f 
>   kdm/backend/client.c a2d06c2 
>   kdm/backend/dm.h b2f8c61 
>   kdm/backend/dm.c 77a2ef7 
>   kdm/backend/dpylist.c b650c2f 
>   kdm/backend/resource.c 38a8c70 
>   kdm/backend/server.c d8dd6f3 
>   kdm/backend/session.c 0e7901c 
>   kdm/config-kdm.h.cmake 3e8912d 
>   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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140526/417f09e7/attachment.htm>