Re: Review Request 112294: Implement multi-seat support in KDM
--- 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
Re: Review Request 117044: Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
On May 19, 2014, 12:05 p.m., Frank Reininghaus wrote:
We are seeing quite a few bug reports about a severe regression between KDE
SC 4.13.0 and KDE SC 4.13.1:
https://bugs.kde.org/show_bug.cgi?id=334776
According to the reporter of https://bugs.kde.org/show_bug.cgi?id=334988
the regression has been caused by this commit.
I'm surprised that this was committed to KDE/4.13 at all - the review
request was for 'master', and it does not really fix a serious bug. Please
make sure that such patches, which fix little annoyances but have the
potential to cause serious trouble, get a lot of testing in betas and RCs
before they are shipped. Testing on a single system is definitely not
enough!
I propose to revert this patch - maybe a better solution which prevents
automounting can be found for master/frameworks, but IMHO we should not
take any further risks in KDE/4.13. Any objections?
Thomas Lübking wrote:
The patch does somehow not what it promises.
KMountPoint::Ptr mp = KMountPoint::currentMountPoints().findByPath( path
);
KMountPoint::currentMountPoints() promises to return mtab, ie. already
used mountpoints, so if there's mp, it's mounted, iow. it looks like (i've
just looked at this for the first time) as if it only shortcuts for already
mounted autofs mounts, but will still statvfs on unmounted autofs mounts
(mounting them implicitly)
Ie. what the patch probably should do was to:
if (!mp) { // unmounted, avoid automounts
KMountPoint::Ptr pmp =
KMountPoint::possibleMountPoints().findByPath(path);
if (pmp pmp-mountType() == QLatin1String(autofs))
return info;
}
I think the submitter took David's suggestion and changed the original
implementation without understanding the consequences. Thomas' suggested patch
would probably be a better starting point.
David: I could not find a isDirectoryMounted() method in kfileitem.cpp to see
what you meant by looking at that implementation.
In the meantime I think this patch should be reverted because it caused a
regression.
- Dawit
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117044/#review58143
---
On May 6, 2014, 8:01 p.m., Tomáš Trnka wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117044/
---
(Updated May 6, 2014, 8:01 p.m.)
Review request for kdelibs.
Repository: kdelibs
Description
---
Avoid unnecessary automounting in KDiskFreeSpaceInfo::freeSpaceInfo
Previously, all unmounted autofs mountpoints were always mounted
by krunner/plasma-desktop on startup, defeating the purpose of
automounting. Let's ignore the unmounted filesystems instead when
gathering free space stats, just like the df utility does.
Everything still gets mounted properly on first real access.
The change itself is pretty trivial and I would regard it as a bugfix
(and thus eligible for 4.13), but I'm posting it for review to see
what you kdelibs people think.
Diffs
-
kio/kfile/kdiskfreespaceinfo.cpp f11eb0998f0e718e9b366f8c26da30586bfa44ef
Diff: https://git.reviewboard.kde.org/r/117044/diff/
Testing
---
I'm using this patch on kdelibs since 4.11 and I have noted no problems
in connection with ~4 automounted filesystems.
Thanks,
Tomáš Trnka
Re: Review Request 112294: Implement multi-seat support in KDM
On May 24, 2014, 11:46 a.m., Stefan Brüns wrote:
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?
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:
---
StaticServers=:0,:1,:2_@seat-foo
ReserveServers=:3,:4,:5_@seat-foo
[X-:*_@seat-foo-Core]
ServerArgsLocal=-layout Seat-Foo
---
and these seats: 'seat0', 'seat-some-machine-generated-specifier', 'seat-foo' --
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+_@seat.* 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
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/112294/#review58394
---
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
Re: Review Request 118270: [doc] explicitly load external entities (after CVE-2014-0191)
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118270/#review58411 --- Thanks for the fix, it seems to work fine. I built KDE SC 4.13.1 (Mageia Cauldron) with it with both original and patched libxml2; in the 2 cases, results are the same, and the same as original meinproc4 with unpatched libxml2. Do we need ressources from network? If all the resources are supposed to be on the local machine, perhaps we should use XML_PARSE_NONET (Forbid network access) option? It is often suggested/recommended to use this option with DTDLOAD and NOENT when it's possible. https://bugzilla.redhat.com/show_bug.cgi?id=863166#c3 - Luc Menut On May 23, 2014, 8:24 p.m., Luigi Toscano wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/118270/ --- (Updated May 23, 2014, 8:24 p.m.) Review request for Documentation, KDE Frameworks, kdelibs, Rohan Garg, Jonathan Riddell, and Rex Dieter. Bugs: 335001 http://bugs.kde.org/show_bug.cgi?id=335001 Repository: kdelibs Description --- Use the more modern API function for XML loading and enable the flags which load the external entities, so that meinproc4 can work again after the security changes implemented for CVE-2014-0191. Without this change meinproc4 complains (see the referenced bug) The fix (half of the patch, the other half is on code which was removed) applies to KF5 too, hence the group. My tests shows that the documentation cache is properly generated as before, and the patch should work even on the old Packagers (Ubuntu packagers in CC, as Ubuntu is one of the few distributions where libxml2 has been already patched) could you please test it with a fixed libxml and without, and if possible with KF5 as well? Diffs - kdoctools/meinproc.cpp 0894d63 kdoctools/xslt.cpp a7265ca Diff: https://git.reviewboard.kde.org/r/118270/diff/ Testing --- meinproc4 works again Thanks, Luigi Toscano
