Add "perpetual" unit concept, sysctl fixes, networkd fixes, systemctl color fixes, nspawn discard by poettering · Pull Request #4481 · systemd/systemd

poettering · 2016-10-25T10:23:04Z

Various smaller improvements

keszybz · 2016-10-27T00:37:46Z

src/core/mount.c

        return 1;
 }

+static void synthesize_root_mount(Manager *m) {


Shouldn't this return an int? Is seems like a failure here should cascade up to the caller of mount enumerate. And you'd avoid the ugly { log_oom(); return; } combos.

i figure synthesize_root_mount() could return an int, indeed. will fix.

mount_enumerate() is a vtable function however. It definitely makes sense to make it return an int too, but that would mean we'd have to change the unit vtable first, and alter all unit type implementations. Makes a lot of sense, but is more invasive, hence let's leave that for a later PR...

keszybz

Yeah, I think that's a reasonable change. But it doesn't solve the problem completely, because you still end with a log of spurious log_notices in a container. Containers otherwise boot nice and quiet, so this is noticeable.

I think we should (at some point in the future), add a way to mark certain sysctl.d files with conditions, similarly to units.

@virtualization=!yes

or so, by taking ConditionX and removing the Condition prefix. Not sure about the exact syntax.

keszybz · 2016-10-27T00:50:21Z

src/sysctl/sysctl.c

@@ -252,7 +252,7 @@ int main(int argc, char *argv[]) {



Then we need = NULL here.

keszybz · 2016-10-27T01:06:12Z

src/nspawn/nspawn.c

+                l = path_startswith(what, "/dev");
+                if (l && startswith(l, "loop"))
+                        options = "discard";
+        }


Should we instead call stat on what? Comparing paths seems rather awkward.

So, what's the other option? checking stat() and the major/minor? So far we tried to avoid hardcoding major/minors, so that the kernel can assign them entirely dynamically as it likes (it does that only for some subsystems so far, but we really shouldn't hardcode for which one it does that and for which one it doesn't, as that might change...).

So far the preferable way to identify the type of a device was either by the subsystem string (which doesn't work here, it's always "block") or by prefix checking of the device name, which is what we do here...

Yes, doing such a prefix name check isn't pretty, but I really see no better way to handle this. All the other options I see are much worse...

keszybz · 2016-10-27T01:09:43Z

Reviewed. One uninitialized mem access, and a couple of questions…

Maybe you should split out the last two patches for networkd into a separate PR? They are rather unrelated and could go in faster.

A pendant for systemd#4481.

evverx · 2016-10-28T00:15:04Z

src/sysctl/sysctl.c

+                         * usually protected their sysctls.) In all other cases log an error and make the tool fail. */
+
+                        if (IN_SET(k, -EPERM, -EACCES, -EROFS, -ENOENT))
+                                log_notice_errno(k, "Couldn't write '%s' to '%s', ignoring: %m", value, property);


Hm, this makes sense inside the container. But I think we shouldn't ignore these errors on the host.
Anyway, #4425 (comment)

-bash-4.3# systemctl --state=failed --no-legend --no-pager systemd-sysctl.service loaded failed failed Apply Kernel Variables

is better than a "clean" boot-up

I think having /proc/sys read-only (either partially or full) should be considered independent of running in a container in the host: it just means "your system shall not be able to make changes to these settings". I think a setup where a host initrd mounts /proc/sys read-only after applying some initial settings, and then handing control over to the host PID 1 is an absolutely OK usecase, that should be handled the exact same way as if we'd run in a container and a container manager had set it up like that for us: it just means "some higher power doesn't want you to touch this, live with it".

Hence, I am pretty sure we should not add an explicit container check here, or handle things differently here if we run in a container: we should just consider this the exact same case.

@poettering , that's about EROFS.
Hm, what about EACCESS/EPERM? Maybe, that's OK to ignore these errors inside the container: #4154
But I'm not sure about EACESS/EPERM on the host

@evverx well, I really wouldn't make a distinction here between EROFS/EPERM/EACCESS, which one you get with this is simply result of the security mechanism you pick. You can use r/o bind mounts, which means EROFS, but you could also implement something like this with selinux/aarmoer/smack/some future LSM or anything else. I think which one you use and hence which kind of permission check you see is an implementation detail and shouldn't alter our handling of it...

A pendant for #4481.

poettering · 2016-11-02T17:06:48Z

or so, by taking ConditionX and removing the Condition prefix. Not sure about the exact syntax.

Hmm, not sure I like that too much. Maybe instead add "-" or so as additional line prefix, that follows the scheme we have with "-" in ExecStart= and suchlike, i.e. suppresses errors triggered by this line entirely.

…l" flag So far "no_gc" was set on -.slice and init.scope, to units that are always running, cannot be stopped and never exist in an "inactive" state. Since these units are the only users of this flag, let's remodel it and rename it "perpetual" and let's derive more funcitonality off it. Specifically, refuse enqueing stop jobs for these units, and report that they are "unstoppable" in the CanStop bus property.

Now that have a proper concept of "perpetual" units, let's make the root mount one too, since it also cannot go away.

…ad-only Let's make missing write access to /proc/sys non-fatal to the sysctl service. This is a follow-up to 411e869 which altered the condition for running the sysctl service to check for /proc/sys/net being writable, accepting that /proc/sys might be read-only. In order to ensure the boot-up stays clean in containers lower the log level for the EROFS errors generated due to this.

This way, we can get rid of a label/goto.

Let's only check for eof once after the fgets(). There's no point in checking EOF before the first read, and twice in each loop.

Let's place only one ternary operator.

If we turn on red color for the active column and it is not combined with underlining, then we need to turn it off explicitly afterwards. Do that.

Make the underlining between the header and the body and between the units of different types span the whole width of the table. Let's never make the table wider than necessary (which is relevant due the above). When space is limited and we can't show the full ID or description string prefer showing the full ID over the full description. The ID is after all something people might want to copy/paste, while the description is mostly just helpful decoration.

Let's make sure that our loopback files remain sparse, hence let's set "discard" as mount option on file systems that support it if the backing device is a loopback.

Fixes: systemd#3870

Let's propagate the error here, instead of eating it up early. In a later change we should probably also change mount_enumerate() to propagate errors up, but that would mean we'd have to change the unit vtable, and thus change all unit types, hence is quite an invasive change.

poettering · 2016-11-02T17:46:09Z

i force pushed a new, rebased version now with the two changes made. Please review and merge!

poettering added network nspawn pid1 systemctl sysctl labels Oct 25, 2016

poettering mentioned this pull request Oct 25, 2016

IPv6 Nameservers are not purged when changing wifi networks #3870

Closed

2 tasks

keszybz reviewed Oct 27, 2016

View reviewed changes

keszybz requested changes Oct 27, 2016

View reviewed changes

keszybz reviewed Oct 27, 2016

View reviewed changes

keszybz added a commit to keszybz/systemd that referenced this pull request Oct 27, 2016

tests: clarify test_path_startswith return value

069395f

A pendant for systemd#4481.

keszybz mentioned this pull request Oct 27, 2016

tests: clarify test_path_startswith return value #4508

Merged

evverx mentioned this pull request Oct 27, 2016

sysctl: run sysctl service if /proc/sys/net is writable #4425

Merged

evverx reviewed Oct 28, 2016

View reviewed changes

keszybz added a commit that referenced this pull request Oct 30, 2016

tests: clarify test_path_startswith return value (#4508)

0470289

A pendant for #4481.

poettering added 13 commits November 2, 2016 11:29

core: make the root mount perpetual too

11222d0

Now that have a proper concept of "perpetual" units, let's make the root mount one too, since it also cannot go away.

unit: unify some code with new unit_new_for_name() call

a581e45

sysctl: split out condition check into its own function

9c37b41

This way, we can get rid of a label/goto.

sysctl: when failing to process a config line, show line nr

98bf501

sysctl: no need to check for eof twice

4f14f2b

Let's only check for eof once after the fgets(). There's no point in checking EOF before the first read, and twice in each loop.

sysctl: minor simplification

e510508

Let's place only one ternary operator.

systemctl: properly turn off color after active column

835a19e

If we turn on red color for the active column and it is not combined with underlining, then we need to turn it off explicitly afterwards. Do that.

nspawn: if we set up a loopback device, try to mount it with "discard"

2bce2ac

Let's make sure that our loopback files remain sparse, hence let's set "discard" as mount option on file systems that support it if the backing device is a loopback.

netword: minor memory leak fix

02affb4

networkd: flush DNSSL/RDNSS lists when we lose carrier

c69305f

Fixes: systemd#3870

poettering force-pushed the perpetual branch from 46210bc to 1201cae Compare November 2, 2016 17:45

keszybz approved these changes Nov 3, 2016

View reviewed changes

keszybz merged commit 7fa6328 into systemd:master Nov 3, 2016

poettering mentioned this pull request Nov 24, 2016

systemd stores incorrect source path for mount unit #3195

Closed

Uh oh!

Conversation

poettering commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keszybz commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evverx Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Nov 2, 2016

Uh oh!

poettering commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

evverx Nov 2, 2016 •

edited

Loading