Nspawn arguments parsing and man page update by keszybz · Pull Request #4319 · systemd/systemd

keszybz · 2016-10-09T15:57:19Z

No description provided.

The documentation says lists "yes", "no", "pick", and numeric arguments. But parse_boolean was attempted first, so various numeric arguments were misinterpreted. In particular, this fixes --private-users=0 to mean the same thing as --private-users=0:65536. While at it, use strndupa to avoid some error handling. Also give a better error for an empty UID range. I think it's likely that people will use --private-users=0:0 thinking that the argument means UID:GID.

evverx · 2016-10-09T16:40:06Z

man/systemd-nspawn.xml

+        <para>Note that <option>-U</option> is the default if the <filename>[email protected]</filename> template unit
+        file is used.</para>
+
+        <para>Note: it is possible to undo the effect of <option>--private-users-chown</option> (or <option>-U</option>)


@keszybz , thanks! I'll test this PR tomorrow.

I think it's not clear what does undo the effect of --private-users-chown (or -U) mean. The full redo is:

machinectl stop cont (or ^]]])

systemd-nspawn --private-users=0

machinectl stop cont (or ]]])

systemd-nspawn (without -U)

BTW: I think it would be great to run something

systemd-nspawn --uidshift ... --dont-spawn-container

Similar to fuidshift :-)

Considering how simple this is, I don't see a good reason not to do this. I only wonder what the best way to expose this would be. IMHO it should be a separate command-like option that would take the same argument as --private-users. I'm only not sure whether this should be in systemd-nspawn or in one of the other tools.

I'm only not sure whether this should be in systemd-nspawn or in one of the other tools

Maybe, systemd-fuidshift ? :-)

evverx · 2016-10-09T21:26:07Z

src/nspawn/nspawn.c

                                arg_uid_shift = UID_INVALID;
                                arg_uid_range = UINT32_C(0x10000);
-                        } else if (r > 0) {
+                        } else if (!optarg || streq(optarg, "yes")) {


Indeed. --private-users=0 works fine. Thanks!
But --private-users=[y|n|true|false|t|f|on|off] is not working. I'm not sure we should break this.
Looks good otherwise

The man page explicitly lists "yes" and "no". It does not say "a boolean". I think that accepting all kind of booleans except for the numerical ones would only increase chances of confusion.

We do this "extended" boolean logic all the time, I think we should do the same here. i.e. first parse the parameter as something other, and if that fails as true boolean with our usual parse_boolean() call... I'd agree with @evverx, we should also be able to use --private-users=true here...

Now that [email protected] includes -U, more users might be interested in this tidbit ;)

evverx · 2016-10-10T00:26:10Z

TEST RUN: Job-related tests
+ /usr/bin/qemu-system-x86_64 -smp 1 -net none -m 512M -nographic -kernel /boot/vmlinuz-4.4.0-38-generic -drive format=raw,cache=unsafe,file=/var/tmp/systemd-test.jJXY6X/rootdisk.img -initrd /initrd.img -append 'root=/dev/sda1 raid=noautodetect loglevel=2 init=/lib/systemd/systemd ro console=ttyS0 selinux=0 systemd.unified_cgroup_hierarchy=no  '
...
TEST RUN: Job-related tests [FAILED]

This is definitely unrelated to this PR (and logs are OK, hm). I'm merging.

poettering

Hmm, I added three more comments. The strndupa() issue really matters and needs fixing.

poettering · 2016-10-10T13:28:58Z

src/nspawn/nspawn.c

                                arg_uid_shift = UID_INVALID;
                                arg_uid_range = UINT32_C(0x10000);
-                        } else if (r > 0) {
+                        } else if (!optarg || streq(optarg, "yes")) {


We do this "extended" boolean logic all the time, I think we should do the same here. i.e. first parse the parameter as something other, and if that fails as true boolean with our usual parse_boolean() call... I'd agree with @evverx, we should also be able to use --private-users=true here...

poettering · 2016-10-10T13:30:21Z

src/nspawn/nspawn.c

-                                        if (!buffer)
-                                                return log_oom();
-                                        shift = buffer;
+                                        shift = strndupa(optarg, range - optarg);


This is no OK, you may not call strndupa() (which means alloca()) in a loop, but we are in a while loop around getopt...

oh, I missed that. sorry

poettering · 2016-10-10T13:33:06Z

src/nspawn/nspawn.c

+                                        if (r < 0)
+                                                return log_error_errno(r, "Failed to parse UID range '%s': %m", range);
+                                        if (arg_uid_range == 0)
+                                                return log_error_errno(EINVAL, "UID range cannot be 0.");


So far we tried to log about error numbers we receive from some call, but not about error numbers we generate (never forget that log_error_errno() attaches an ERRNO= field to the journal entry for the msg). I think this should hence be written as:

if (arg_uid_range == 0) { log_error("UID range cannot be 0."); return -EINVAL; }

Is this a reason why do we use so many:

log_*(...) return -*

in nspawn?

@evverx well, depends. basically, I think we should never do log_error_errno() and then specify a literal as first argument. That's all. Because in that case we are generating the error code, not receiving it. And I doubt we should log about that. It's the caller which should (or should not) log about the error code then...

It's not so obvious. I think it would be useful to add this rule to the CODING_STYLE

Indeed, will add something.

I know we have this rule, but I'm not convinced it's right. After all

return log_error_errno(EINVAL, "…");

is much more concise and thus much clearer then

{ log_error("…"); return -EINVAL; }

I'd say that the practicality of the short form beats the purity of the long one.

Indeed, instead of setting this rule in stone, I'd rather rescind it, and allow judicious use of the shortened form with the hardcoded return errno. The only time where it should not be imo allowed, would be when the errno value makes no sense (i..e. when we return ESRCH or ENOMEDIUM because we need to pass some state to the caller and we use a fake errno that is special-cased in the caller).

keszybz added 2 commits October 9, 2016 11:51

nspawn: reindent table

27eb8e9

keszybz added nspawn documentation labels Oct 9, 2016

keszybz mentioned this pull request Oct 9, 2016

nspawn: change the synced cgroup hierarchy permissions too #4223

Merged

evverx reviewed Oct 9, 2016

View reviewed changes

man: describe how to reverse systemd-nspawn -U

6265bde

Now that [email protected] includes -U, more users might be interested in this tidbit ;)

keszybz force-pushed the nspawn-arguments branch from d6d7b16 to 6265bde Compare October 9, 2016 22:26

evverx merged commit 7633689 into systemd:master Oct 10, 2016

keszybz deleted the nspawn-arguments branch October 10, 2016 00:31

poettering reviewed Oct 10, 2016

View reviewed changes

poettering added the merged-but-needs-fixing label Oct 10, 2016

keszybz mentioned this pull request Oct 10, 2016

nspawn --private-users parsing, v2 #4332

Merged

evverx mentioned this pull request Oct 11, 2016

udevadm: use parse_sec instead of atoi #4331

Merged

Uh oh!

Conversation

keszybz commented Oct 9, 2016

Uh oh!

evverx Oct 9, 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!

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!

evverx commented Oct 10, 2016

Uh oh!

poettering 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!

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

evverx Oct 9, 2016 •

edited

Loading