Skip to content

Fix logic for R.#1

Closed
vaeth wants to merge 1 commit intoOpenRC:masterfrom
vaeth:master
Closed

Fix logic for R.#1
vaeth wants to merge 1 commit intoOpenRC:masterfrom
vaeth:master

Conversation

@vaeth
Copy link
Copy Markdown
Contributor

@vaeth vaeth commented Mar 8, 2017

It should not be an error ($? != 0) to apply R to a nonexistent path.
Without this fix, tmpfiles refuses to start when systemd-233 is installed,
because this provides config files containing lines like
R /tmp/systemd-private-* 0 0 0

It should not be an error ($? != 0) to apply R to a nonexistent path.
Without this fix, tmpfiles refuses to start when systemd-233 is installed,
because this provides config files containing lines like
R /tmp/systemd-private-*  0 0 0
@lu-zero
Copy link
Copy Markdown

lu-zero commented Mar 8, 2017

What does it do currently and what does it do after your change?

@vaeth
Copy link
Copy Markdown
Contributor Author

vaeth commented Mar 8, 2017

Very verbosely:

Without the patch, the opentmpfiles.setup service in openrc refused to start (i.e. it reports a failure), causing all dependent service to not start either, and thus not booting properly.

Analysis of the problem revealed: opentmpfiles.setup calls tmpfiles (with some args), and the latter returns with exit status 2 instead of 0. Further analysis showed that the exit status nonzero is caused by the 2 lines in /usr/lib/tmpfiles.d/tmp.conf (which is installed by systemd-233):

R! /tmp/systemd-private-*
R! /var/tmp/systemd-private-*

In my opinion, the non-existence of the directories to be removed should not trigger an error.
However the return status (i.e. $?) after the expression
[ -d "$path" ] && ...
is nonzero if "$path" is not a directory. Inverting the logic
! [ -d "$path" ] || ...
returns status 0 if "$path" is not a directory: The latter is a short form of

if [ -d "$path" ]
then ...
fi

After the patch, tmpfiles exits on my system with status 0, and so opentmpfiles.setup (and its dependent services) come up successfully, booting my machine correctly with openrc.

@williamh
Copy link
Copy Markdown
Contributor

williamh commented Mar 8, 2017

I'll take a look at this when I get to the office in a little while.

@williamh
Copy link
Copy Markdown
Contributor

williamh commented Mar 8, 2017

Ok, I reproduced your issue. I would rather use the long form of the if statement to fix it however because I want the logic to follow the definition of the command on the man page. I also see multiple other places where this could become an issue, so I'll take care of them as well.

@lu-zero:
This one is pretty subtile, but here's what is going on.

[ foo ] && bar

can return a different return code than

if [ foo ]; then
bar
fi

Both of them will execute bar only if foo is true, but the first one gives you a false return code if foo is false.

@vaeth
Copy link
Copy Markdown
Contributor Author

vaeth commented Mar 8, 2017 via email

williamh added a commit that referenced this pull request Mar 8, 2017
There is a big difference between how these tests behave:

1. [ foo ] && bar
2. if [ foo ]; then bar; fi

The first test will return a failure return code if foo is false, and
this was causing issues, so we need the second test.

This is for #1.
@williamh
Copy link
Copy Markdown
Contributor

williamh commented Mar 8, 2017

@vaeth: Please test with the commit I just pushed to master and report back. The return code isn't fixed yet. I want to handle it the same way systemd-tmpfiles does, so I need information on this since I do not run systemd.

@vaeth
Copy link
Copy Markdown
Contributor Author

vaeth commented Mar 8, 2017

The current master booted without issues so far.

the same way systemd-tmpfiles does

I looked at the source code of tmpfiles.c in systemd. It uses glob_item() to resolve the mask, and that function returns a nonzero error code if for any of the masks the passed function (here: rm_rf()) fails.

Summary: D succeeds in systemd-233 only if all directories matching the mask could be cleaned successfully. (If the mask does not match anything, this also means a success.)

@williamh williamh closed this in 5387349 Mar 8, 2017
williamh added a commit that referenced this pull request Mar 8, 2017
There is a big difference between how these tests behave:

1. [ foo ] && bar
2. if [ foo ]; then bar; fi

The first test will return a failure return code if foo is false, and
this was causing issues, so we need the second test.

This is for #1.
williamh added a commit that referenced this pull request Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants