Review Request 112294: Implement multi-seat support in KDM

Oswald Buddenhagen ossi at kde.org
Fri Mar 28 10:59:40 GMT 2014 

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


the general approach seems sensible (accepting the limitation that secondary seats can have no reserve displays - i guess this should actually work if done right?).


CMakeLists.txt
<https://git.reviewboard.kde.org/r/112294/#comment38033>

    i suspect it would be better to have that in kdm/ConfigureChecks.cmake.



cmake/modules/FindSystemd.cmake
<https://git.reviewboard.kde.org/r/112294/#comment38032>

    i don't think the conditions are necessary in the cmake version required by kde.



kdm/backend/CMakeLists.txt
<https://git.reviewboard.kde.org/r/112294/#comment38034>

    this should go to config-kdm.h.cmake.



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

    this is redundant with the line below.
    if some particular pam module needs this setting, it should be done much earlier than here, and the next line would be pointless (we use pam_getenvlist). that would also mean that pam is a hard dependency, which should be expressed by the configure code and appropriate ifdefs.



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

    that define should be indented as well.



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

    the declarations should NOT be indented.



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

    void parameter list missing (this is not c++).



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

    ditto



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

    kdm's style does not use NULL.



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

    the empty lines around this hunk don't quite fit the surrounding style.



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

    that's more of a logWarn, i think?



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

    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?



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

    excess braces (kdm uses qt style, not kdelibs style).



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

    that can be const char *



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

    please use the conventional 'd'. applies to other places as well.



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

    startReserveDisplay doesn't break, because it wants to find the last unused display (because the list is reversed).



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

    i think that should be "there are not ...".



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

    excess braces.
    
    and excess parens.



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

    pointless



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

    this is a config setting and should not be overridden like that.



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

    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.



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

    uh-oh. you really shouldn't do that.
    first, find only displays which don't have a vt set. then, check for the presence of a seat name instead of calling the vt allocator.
    that way reserve displays and secondary seats will be still mutually exclusive, but at least they shouldn't interfere.



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

    if this is necessary, you are already in deep trouble.



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

    trailing space



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

    i'd call it just "seat".



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

    i prefer !strcmp(). repeats several times.
    
    also, this seems like a hack. it should be possible to properly query whether kdm should manage this display.



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

    should be melted into the condition below.



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

    that belongs one line down.



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

    the duplication in this function shows that this isn't a particularly elegant approach. 
    
    first, call forEachDisplay(markDisplay);
    then iterate over the new names and set stillThere = True.
    finally, iterate over the displays and delete what went byebye.
    



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

    i prefer to put () after functio names.



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

    freeStrArr()



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

    why the redundant supply of the seat as a layout?



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

    attach to closing brace.



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

    is this really necessary? i would expect it to be the default.


- Oswald Buddenhagen


On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/112294/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2013, 11:34 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 a3bdbb3 
>   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 64e106b 
>   kdm/backend/dm.c e0f1366 
>   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/20140328/8cda6a36/attachment.htm>

More information about the kde-core-devel mailing list