DevHeads.net

Review Request 112294: Implement multi-seat support in KDM

Review request for kde-workspace.

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

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: <a href="http://git.reviewboard.kde.org/r/112294/diff/" title="http://git.reviewboard.kde.org/r/112294/diff/">http://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Comments

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 09/02/2013 - 15:57

(Updated Sept. 2, 2013, 7:57 p.m.)

Review request for kde-workspace.

Changes
use Qt coding style

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
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: <a href="http://git.reviewboard.kde.org/r/112294/diff/" title="http://git.reviewboard.kde.org/r/112294/diff/">http://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 09/02/2013 - 19:34

(Updated Sept. 2, 2013, 11:34 p.m.)

Review request for kde-workspace and Oswald Buddenhagen.

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

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: <a href="http://git.reviewboard.kde.org/r/112294/diff/" title="http://git.reviewboard.kde.org/r/112294/diff/">http://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 03/29/2014 - 13:14

(Updated March 29, 2014, 5:14 p.m.)

Review request for kde-workspace and Oswald Buddenhagen.

Changes
Part one: fix trivial issues

Repository: kde-workspace

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
CMakeLists.txt d5c76d8
cmake/modules/CMakeLists.txt 117b3a5
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: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 03/29/2014 - 13:38

(Updated March 29, 2014, 5:38 p.m.)

Review request for kde-workspace and Oswald Buddenhagen.

Changes
readd FindSystemd.cmake (erroneously dropped), one more trivial fix

Repository: kde-workspace

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
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: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 04/06/2014 - 16:16

(Updated April 6, 2014, 8:16 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:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

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: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 04/06/2014 - 18:01

(Updated April 6, 2014, 10:01 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:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
cmake/modules/CMakeLists.txt 117b3a5
cmake/modules/FindSystemd.cmake PRE-CREATION
kdm/ConfigureChecks.cmake b61fd90
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
kdm/config-kdm.h.cmake 3e8912d
plasma/desktop/applets/windowlist/plasma-applet-windowlist.desktop 50c9f66
plasma/desktop/containments/desktop/plasma-containment-desktop.desktop ce9fe38
plasma/generic/wallpapers/image/plasma-wallpaper-image.desktop 6484545

Diff: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/18/2014 - 15:44

(Updated May 18, 2014, 7:44 p.m.)

Review request for kde-workspace and Oswald Buddenhagen.

Changes
Enhanced the patch based on feedback.

The second patch adds a DynamicServers option. Displays are specified like for StaticServers, e.g. "DynamicServers=:10,:11". The seat name can be specified in place of the class name, e.g. "DynamicServers=:10_@seat-left,:11_@seat-right". If the seat name is specified each server can be provided with its own set of options, e.g:

[X-:*_@seat-left-Core]
ServerArgsLocal=-layout seat-left

This patch also fixed two bugs in the config/class matching code. The class name in the display list included the "_" separator, and the class in the section name did not strip the "-" character between class name and subsection name.

Repository: kde-workspace

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
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: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/25/2014 - 20:07

(Updated May 26, 2014, 12:06 a.m.)

Review request for kde-workspace and Oswald Buddenhagen.

Changes
Updated patch. Collapsed patch, because reviewboard does not accept git format-pacht series ...

Repository: kde-workspace

Description
This patch implements dynamic multiseat in KDM. It follows the description in:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
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: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/29/2014 - 15:03

(Updated May 29, 2014, 7:03 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:
<a href="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/" title="http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/">http://www.freedesktop.org/wiki/Software/systemd/writing-display-managers/</a>

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 <a href="https://bugzilla.redhat.com/show_bug.cgi?id=884271" title="https://bugzilla.redhat.com/show_bug.cgi?id=884271">https://bugzilla.redhat.com/show_bug.cgi?id=884271</a> and <a href="https://bugzilla.redhat.com/show_bug.cgi?id=975079" title="https://bugzilla.redhat.com/show_bug.cgi?id=975079">https://bugzilla.redhat.com/show_bug.cgi?id=975079</a>

Diffs (updated)
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/config-kdm.h.cmake 3e8912d
kdm/kfrontend/kdm_config.c 368c8d1

Diff: <a href="https://git.reviewboard.kde.org/r/112294/diff/" title="https://git.reviewboard.kde.org/r/112294/diff/">https://git.reviewboard.kde.org/r/112294/diff/</a>

Testing
Single seat system, several multiseat systems

Thanks,

Stefan Brüns

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/31/2014 - 05:35

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

it would be more logical to have that one above the empty line, imo.

also, you need a registerCloseOnFork() call.

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

that should go into the if for clarity.

more importantly, you also need an unregisterInput() and a clearCloseOnFork(). well, for formal correctness at least.

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

this doesn't reflect what we discussed ...

you are treating no seat being set as a wildcard. this doesn't match the idea that a legacy spec should automatically be assigned to seat0.

the code here could still work, but you'd need to post-process the config, as i have pondered before. probably not a good idea, though.

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

it just occured to me that early exiting the loop is actually the wrong thing to start with: StaticServers=:0,:1 (both implicitly on seat0) would not work this way.

so you need to act upon all exact hits, and fall back to a single wildcard hit.

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

this is actually tricky ... what if the display is still there but the seat changed? i don't think you are handling the transition (which would involve the 'raiser' state, iirc), so the display would come up again only after the next hup or replug.

it's probably not worth it to handle the complexity, but maybe add a comment somewhere.

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

this belongs into the if

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

this being here makes the continue in line 1461 an infinite loop. move it back into the loop header and add a space and semicolon right after the label (cf. line 1406 in upstream).

the StaticServers doc still needs to be extended to reflect the new capabilities.

i pushed the off-by-one fixes so we have that out of the way.

- Oswald Buddenhagen

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 06/02/2014 - 02:32

well, i elaborated on the pros and cons of that approach already ...
i don't really buy into "break X semantics". one could say the same about overloading the class. it's not visible outside kdm, so it doesn't matter.

anyway, as i said, it's not worth the trouble, so close it unless you feel the urge to make it more elegant despite zero practical gain.

- Oswald

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 06/01/2014 - 10:26

This could be done by not using the class attribute, i.e. use @seat-foo:0 instead of :0_@seat-foo. @seat-foo:0 would be the name, and the lookup happens by name. On the other hand, the server startup code would have to handle stripping the seat name before starting a local display.

On the other hand <hostname>:<diplaynumber> has a fixed meaning in the X world, and using a @seat-foo hostname would break X semantics. So I prefer sticking with the class name for the seat.

Issue fixed?

- Stefan

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/31/2014 - 14:24

hmm, right.

note that this is semantically a bit different than changing the class, because it isn't just a minor config parameter. anyway, not worth the trouble to handle it differently.

- Oswald

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/31/2014 - 10:23

Can you please elaborate? As long as a seat is running it is fixed to a seat. Changes to the class specifier for a running display are not handled, but that has always been the case.

- Stefan

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/31/2014 - 09:49

as systemdMonitorDeinit is also called in case of an error I have added both. I also changed the if() to check for systemdMonitorFd >= 0 instead of systemdMonitor. The latter was never set to 0, so in case of too many failed calls to sd_get_seats() systemdMonitorDeinit would be called once, and again during shutdown. A valid systemdMonitorFd implies a valid systemdMonitor.

- Stefan

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/26/2014 - 10:50

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

Yes, leftover, will remove

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

nope, will fix

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

thanks for the clarification, will fix

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

Hm, I prefer this notation, in particular as this is the same as for std::string (guaranteed since C++03, working for all versions of C++):

std::string foo(100, '\0');
char* buf = &foo[10];

vs

char foo[100] = { '\0' };
char *buf &foo[10];

This is correct, as arrays (both C and C++) are guaranteed to be contiguous, and thus (&foo[10]) == (foo + 10).

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

Nope. After systemdHandleChange() we will repeat the while(...) loop, calling:
startDisplays();
which calls:
forEachDisplay(checkDisplayStatus);
which does:
if ((d->displayType & d_origin) == dFromFile && !d->stillThere)
stopDisplay(d);

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

They are:
<a href="http://domainkeys.sourceforge.net/underscore.html" title="http://domainkeys.sourceforge.net/underscore.html">http://domainkeys.sourceforge.net/underscore.html</a>

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

see above

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

Will remove

- Stefan Brüns

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/29/2014 - 15:00

Yeah, plain C sucks. Changed to pointer plus offset.

this is d->seatRemoved now.

- Stefan

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/28/2014 - 04:37

i know how c works. ;)
it still looks wrong from the mathematical pov, because you actually want a slice, kinda &foo[10..inf]. this is better expressed by writing something that looks more like just the starting offset, i.e., foo + 10. it also happens to have less meta char noise.
and yes, this is a matter of preference. my code, my rules. :P

huh? this page specifically bolsters my point. this is about host names, not generic dns entries.

good point. but this means that we actually may not re-use this variable, because it will interfere, no matter what (your code would inadvertently re-animate a just-died display, i think).
i suggest you keep the mechanism, but use a new variable "activated" for it.

- Oswald

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/26/2014 - 04:32

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:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/31/2014 - 09:52

The last one sounds much better, will use it.

- Stefan

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/31/2014 - 04:17

i meant "There is no matching unused dynamic display for %s\n". it's an implicit AND between the attributes.
though i guess the original order is actually better, so "unused matching dynamic display".
or you choose different approach: "No matching dynamic display available for %s\n". it doesn't need to be more verbose than that.

- Oswald

On May 29, 2014, 7:03 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/29/2014 - 15:02

systemd itself requires pam, so this is not an issue.

fixed the other issues

"There is no unused or no matching unused dynamic display for %s" sounds strange IMHO

- Stefan

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/25/2014 - 20:13

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

This is actually needed to supply the seat to logind session registration via pam_systemd.
This could be placed earlier, but I chose this place because it is near the old CK session registration, and it only has to be before pam_open_session(...)

- Stefan Brüns

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/24/2014 - 07:45

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:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/29/2014 - 15:01

DynamicServers has been dropped completely

- Stefan

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 05/25/2014 - 05:03

On May 24, 2014, 11:46 a.m., Stefan Brüns wrote:
yes, you understood me right.
but as you point out correctly, i didn't consider the case of plugging unknown seats. how about this:

StaticServers=:0,@*:1,:2_@seat-foo

:0 is a fixed binding, as no seat is implicitly seat0 - exact match.
seat-foo will get :2 - exact match.
seat-some-machine-generated-specifier would get :1 - wildcard match.

for the b case, you slightly modified my proposal, but your version is actually better suited for the wildcard handling. we just define that a legacy-compatible config must have explicit entries without a seat spec (which default to seat0 with systemd). that way we don't need to exclude seat0 from wildcard matching.

note that in the config section headers, :0 and :0_@seat0 are equivalent.
implementation-wise, that may mean that it is best to null out the seat0 name asap, i.e. when parsing the config and when receiving seat names from systemd. at first sight, that seems to contradict the previous paragraph, but in a way it doesn't ... displays with an explict seat0 spec would just turn into displays with no seat spec. that would have the advantage of needing the fewest conditionals further down the line, which would also mean the fewest #ifdefs.

now comes the next problem: ServerArgs* needs to pass the seat to the server somehow.

[X-:*_@*-Core]
ServerArgsLocal=-seat @SEAT@ -layout @SEAT@

the -layout is actually irrelevant for unknown seats, as they can't have predefined layouts by definition.
i'm not sure yet whether i want to insist on passing -seat via the config. technically, it would be cleaner to take that knowledge about the server command line out of kdm. otoh, it already makes assumptions about how displays and vts are passed, and the seat is quite akin to that. oh, it also knows about auth files. and xdmcp queries. ok, forget the idea - you need no @SEAT@ magic. ^^

- Oswald

On May 18, 2014, 7:44 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/24/2014 - 14:15

On May 24, 2014, 11:46 a.m., Stefan Brüns wrote:
Hm, not exactly sure if I understand your proposal correctly - do you mean get rid of DynamicServers and use the StaticServers for either "legacy" static servers or for seats provided by systemd-logind, dependent on logind availability?

If yes, I am generally for it. But this would also require that the StaticServers list does not require (but allow) any special syntax for seats. Seats may be totally dynamic, added by e.g. plugging a DisplayLink USB adapter, with a name dependent on the used USB port.

So given the following partial config:
In case of compiled in support for systemd-logind all StaticServers would be flagged with status dynamic instead of notRunning.
[a: logind running] seat-foo will use DISPLAY=:2, whereas the other two will use one of :0 and :1, which one is unspecified. :2 will use "-layout Seat-Foo" as args, the other two will use the default args (or section [X-:*-Core])
[b: logind not running] print a warning/error in kdm.log. Change the status from dynamic to notRunning for all seats without a seat specifier, this will spawn displays/greeters for :0 and :1, and if there where [X-:{0,1}-Core] config sections, these would be used.

[c: No logind support compiled in] print a warning for each display in StaticServers/ReserveServers matching :\d+ ... at seat dot * and remove from the Static|ReserveServers list, i.e. :0 and :1 will be started.

In case ReserveServers support is added later, seat-foo would use :5 and the same [X-:*_@seat-foo-Core] config section as display :2.

- Stefan

On May 18, 2014, 7:44 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 03/28/2014 - 06:59

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:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/29/2014 - 15:01

the configure code adds a dependency on PAM if systemd is found/enabled.

- Stefan

On May 26, 2014, 12:06 a.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/18/2014 - 17:58

Actually, pam_systemd needs it to register the session for the correct seat ...

- Stefan

On May 18, 2014, 7:44 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 05/18/2014 - 16:32

Added an option DynamicServers, displayType dDynamic and DisplayStatus dynamicReserve

For backwards compatibility the display for seat0 will only be taken from the DynamicServers list is specified explicitly, e.g. DynamicServers=: ... at seat0 dot This is consistent with how seat0 is handled specially by the X server.

- Stefan

On May 18, 2014, 7:44 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 04/09/2014 - 15:09

the gain is that it makes the system cleaner and more predictable.
it should be really pretty much trivial to implement it (just follow the pattern for ReserveDisplays).

the question is why you are handling seat0 differently to start with.

the obvious solution would seem having an empty StaticServers and pulling :0 from DynamicServers as well.
now the question is what to do if systemd isn't actually there. does kdm have to do anything useful when it is configured with the incorrect assumption of systemd being there? i don't think so, given how central systemd is.

and given the config stuff discussed below, you can have both:
StaticServers=:0_@seat0
DynamicServers=:3_@seat1
i.e., kdm manages :0 as belonging to a seat, and if systemd *also* reports it, then kdm can ignore it without any hard-coded special cases.
not sure about automatically passing the -seat option to static servers declared this way - what does a server do in such a case if there is no systemd running?

reserve displays on secondary seats don't need to be implemented now, but it makes sense to consider them in the design to the degree i already did.
the config stuff should be done properly from the start.

- Oswald

On April 6, 2014, 10:01 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 04/07/2014 - 09:47

1. changed to !strcmp()

2. with the current patch all seats but seat0 which also have the CanGraphical property are picked up by KDM. Although this is not ideal, it is still a major improvement to the current situation. Currently I can not see any use case for a graphical seat not managed by KDM, but I see a use case for multiseat in general.
In general this can be achieved if one extends the patch set with the extended configuration mentioned below (using display classes).

I would prefer to keep the current patch set as is.

Currently I have not implemented (2a), but still use (1). I still can do that, although I see no apparent gain here until there is a possibility to configure individual seats. If this is really required I will do that, but this will take another week (I can only do this in my spare time ...)

IMHO this is material for a second patchset. Secondary seats with reserve displays will require much more integration with logind, and this will be major surgery. Auto-login on secondary seats would be nice to have (and you can use it as long as you only have one secondary seat in addition to seat0), but I prefer to postpone this.

- Stefan

On April 6, 2014, 10:01 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 04/06/2014 - 16:20

Its not necessary (and the code path in the xserver -- include/hotplug.h treats SeatID == 0 and SeatId == "seat0" the same), and it will break pre 2011 Xorg, so I will drop this hunk.

- Stefan

On April 6, 2014, 8:16 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 03/31/2014 - 05:54

i would prefer 2.a. it shouldn't be much work.
i had shortly pondered 2.b myself, but then you'd need a new factory concept, and the configuration would be different, so i discarded it.

hrmpf. again somebody did half a job, and others need to make workarounds to make it usable.

it may be actually possible to integrate this nicely with kdm's super-complex config system.
it already supports a display-identifying mechanism, the so-called display class. you can have groups that look like [X-*:*_foos-Core]. you can even have StaticServers=:0_foos,:1_foos,:3_bars.

one way to make use of this would be simply replacing the display class with @seat1 or something like that. the actual display class is pretty much meaningless for local displays anyway (it was introduced to group remote (predominantly xdmcp) displays, which could have different physical properties for example).

another way would be making local displays look like @seat1:1. that makes a lot of sense - the seat kinda is the "host", even if of a different kind. this approach might be a bit more complex, though (it would need special handling of the host part, while just replacing the display class would be practically zero effort).

that way one could even have static secondary seats (something people had hacked a decade ago already). and it would have potential for supporting reserve displays on secondary seats (ReserveServers=:1,:2,:3,@seat1:5,@seat1:6). DynamicServers could use both static (@seat1:4,@seat2:7) and dynamic (:4,:7) seat to display assignment.
if the display class based approach was chosen, this would be :4_@seat1 and so on.

this seems quite complex from the user perspective, but the default set would be pretty static, so one can put it into config.def to have genkdmconf produce the common skeleton. at some point we'd remove the template, and people could remove it from their old configs (as opposed to leaving it in forever or making a hard cutoff, which would be the options if -layout was hard-coded in kdm itself).

note that integration with the config system is sort of necessary anyway - for example to make auto-login configurable on secondary seats.

- Oswald

On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 03/30/2014 - 14:23

The seat option is used for matching devices from udev, i.e. input devices, GPUs, ... It specifies *which* devices to use, but now *how to configure* these.

Unfortunately the config files lack a "MatchSeat" option, so the only possible options are either specifying a config file (or directory) or a layout. There is a patch from Oleg Samarin floating around which adds MatchSeat, but this is not upstream so currently no option.

If a layout is specified but not available, the default layout is chosen, so no problem here.

ServerArgs is no alternative as it is specific to a display, but the seats will use a more or less random display, dependent on when they become available (much like Reserve servers, which take the first display available).

So, what approach to take here:
1. Its somewhat ugly, but it works, so use it.
2. Introduce a dDynamic display type, and make it behave like dPermanent.
a) add a DynamicServers option for this
b) create list of displays on the fly.

Reserve displays on dynamic seats is not possible currently, you cannot change the VT. For this to work you need support for systemd-login, but this is more serious surgery.

- Stefan

On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 03/30/2014 - 04:52

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

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.

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

On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 03/29/2014 - 15:54

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.

- Stefan

On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 03/29/2014 - 14:08

Regarding error codes, the calls are documented as:
"On failure, these calls return a negative errno-style error code."

There are no config matches taking the "seat" into account, so the only possibility to apply a specific config is to use "layout".

- Stefan

On March 29, 2014, 5:38 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 09/03/2013 - 18:20

given that there is no intention to make further feature releases of the kde workspace which will include kdm, i wonder why we'd go through the (potentially tedious) process of upstreaming this now?

- Oswald Buddenhagen

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 03/27/2014 - 18:37

Ok, this is in Fedora for quite some time, and has been accepted to openSUSE two weeks ago. In openSUSE it will appear as part of the KDE:Release:4.12 and KDE:Release:4.13 addon repos, and will be included in openSUSE 13.2, due in august.

- Stefan

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Aaron J. Seigo at 02/18/2014 - 21:32

"maintenance releases with new features?"

long term support, not purely maintenance; the idea is to have a limited-changes, maintained version of the desktop shell based on Qt4 while we move to Qt5 and all that brings with it. the 4.11 branch needs to remain usable and relevant over that time period otherwise the purpose is defeated. if distributions migrate to systemd (which they obviously are) and there are quality downstream patches to improve integration there, it makes sense to fold those into the long term release branch.

"i can make an initial review as time permits."

that would be awesome and greatly appreciated.

cheers ...

- Aaron J.

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 02/18/2014 - 18:12

maintenance releases with new features? again somebody trying to eat the cake and have it, huh? ;)

anyway, if there is commitment to this feature, i can make an initial review as time permits.

- Oswald

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Aaron J. Seigo at 02/18/2014 - 11:03

There is ~1.5 years of releases of kde-workspace 4.x left to come. They are maintenance releases, but releases all the same. So yes, upstreaming this would see the light of day as a 4.11.x release.

- Aaron J.

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 09/06/2013 - 03:29

provided any distro still wants to make a new feature release.
anyway, you'll understand that my motivation to invest effort into this is kinda low, time constraints notwithstanding.
i may reconsider if i see credible support for such a patch from multiple downstreams here.

- Oswald

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Martin Sandsmark at 09/05/2013 - 14:43

Well, at least it gives distros somewhere to pick the patch from.

- Martin Tobias Holmedahl

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Oswald Buddenhagen at 09/05/2013 - 02:35

that's besides the point. whatever gets merged now will never be released. i'm not quite sure why the responsible persons didn't rm -rf the directories yet.

- Oswald

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?q?Stefa... at 09/04/2013 - 14:16

The reason for sending this was to have one canonical implementation for multiseat support which is upstream.
Otherwise, any patches/bugreports must be coordinated downstream, which I really dislike.

Reason for pushing this into KDM is that:
a) KDM is here today and will stay for some time
b) this patch has been tested thoroughly
c) alternative DMs are not up to the job yet (SDDM) or introduce additional dependencies (GDM)
d) I want multiseat support in the DM now, not in a distant future

- Stefan

On Sept. 2, 2013, 11:34 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By Martin Sandsmark at 09/02/2013 - 18:30

I don't see anything obviously wrong with it, but you should probably explicitly add Oswald Buddenhagen as a reviewer.

- Martin Tobias Holmedahl Sandsmark

On Sept. 2, 2013, 7:57 p.m., Stefan Brüns wrote:

Re: Review Request 112294: Implement multi-seat support in KDM

By =?utf-8?b?TWFyd... at 08/27/2013 - 11:26

Hi, although I'm not entirely convinced this patch's quality is good enough to be actually upstreamed (and I didn't intend to do it since the beginning), the fact SUSE adopted it, it is being used and there is active interest in its further development, I will support its inclusion.
I don't really understand why to include it now though as kde-workspace is being frozen at 4.11 and KDM was being said to be obsoleted for Plasma Workspaces 2. Could please anybody explain if this does relate? Thanks.

- Martin Bříza

On Aug. 26, 2013, 3:49 p.m., Stefan Brüns wrote: