Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for opening files for appending #9500

Merged
merged 1 commit into from Jul 20, 2018

Conversation

@zsol
Copy link
Contributor

commented Jul 3, 2018

This is a reimplementation of #8984 based on the end result of the discussion over there.

Addresses part of #8983

@davide125

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2018

The Rawhide CI failure here looks unrelated

@poettering

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

Hmm, the most recent ideas discussed in #8984 were about creating "create" rather than "trunc", i.e. doing open(O_CREAT) with a temporary name, and then replacing any existing file instead of O_TRUNC, so that we definitely get a new inode. (even better: use O_TMPFILE and linkat() if that works, to avoid the temporary name)

@poettering

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

patch looks pretty OK otherwise.

(note though that we usually prefer merging "perfect" PRs, i.e. the commits that make up a PR should reflect logical steps, not historical ones, hence please squash any commits that just fix mistakes of earlier ones in the same PR)

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

Thanks for the feedback, I'll iterate on this soon (truncate -> create with the tempfile semantics, hopefully avoiding the name).

please squash any commits that just fix mistakes

I'm happy to do this if you prefer it like that. On my other open source projects I just let github do the squashing, because I find reviewing small incremental changes to the PR easier.

@keszybz

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

I just let github do the squashing

The PR should be composed of as many logical patches as needed, each one doing a separate thing. Github can only squash everything into one patch, but if more than one patch is wanted, it cannot help with that.

@keszybz

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

… and even if there's just one patch, the "squash & merge" option sometimes mangles patch authorship. So it is overall better if you do the squashing yourself.

facebook-github-bot added a commit to facebookincubator/rpm-backports that referenced this pull request Jul 5, 2018

rpms: backport a few more systemd PRs
Summary:
- backport systemd/systemd#9460 and systemd/systemd#9500
- revert systemd/systemd@c58fd46 to workaround T30958493

Reviewed By: zsol

Differential Revision: D8731646

fbshipit-source-id: 718eacbf2480f7ae948652e193ee3238c2426fdb

@zsol zsol force-pushed the zsol:append branch from b3c2f66 to 9a4673c Jul 9, 2018

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

I reworked truncation (and renamed it to create: because it seemed more accurate) and fixed all the tests so they actually fail if something breaks :)

Unfortunately I couldn't use O_TMPFILE as the only way to give it a name is with linkat, which won't overwrite existing files. I went with mkostemp and renameat instead, but unfortunately this doesn't seem to work either. The file is properly created, but I can't seem to get any output in it. I'd appreciate some help here :)

I left in some debugging in the create test case. This is the output I currently get from it:

output(0): x
expected(6): hello
x
Assertion 'streq(output, expected)' failed at ../src/src/test/test-execute.c:643, function test_exec_standardoutput_create(). Aborting.

If I just open the output target with O_TRUNC instead of all that tempfile dance, I also get the same behavior (i.e. file is truncated but no output is sent there)

@keszybz
Copy link
Member

left a comment

It turns out we have a bigger problem. The output and input configuration is applied to all ExecStartPre and ExecStart commands.

This is easily seen with the following units:

# /etc/systemd/system/outputtest-data.service
[Service]
StandardInputText=asdf asdf
StandardInput=data

ExecStartPre=awk '{print "pre:" $0; }'
ExecStart=awk '{print "main:" $0; }'
ExecStartPost=awk '{print "post:" $0; }'

StandardOutput=journal

which produces

1531231856.507479 systemd[1]: Starting outputtest-data.service...
1531231856.513393 awk[18922]: pre:asdf asdf
1531231856.518742 awk[18923]: main:asdf asdf
1531231856.519105 awk[18924]: post:asdf asdf
1531231856.522894 systemd[1]: Started outputtest-data.service.

and

# /etc/systemd/system/outputtest-tty.service
[Service]
ExecStartPre=ls -l /proc/self/fd/
ExecStart=ls -l /proc/self/fd/
StandardInput=tty
TTYPath=/dev/tty6
StandardOutput=journal

which produces

1531231911.241540 systemd[1]: Starting outputtest.service...
1531231911.248787 ls[18975]: total 0
1531231911.248787 ls[18975]: lrwx------. 1 root root 64 Jul 10 16:11 0 -> /dev/tty6
1531231911.248787 ls[18975]: lrwx------. 1 root root 64 Jul 10 16:11 1 -> socket:[2845117]
1531231911.248787 ls[18975]: lrwx------. 1 root root 64 Jul 10 16:11 2 -> socket:[2845117]
1531231911.248787 ls[18975]: lr-x------. 1 root root 64 Jul 10 16:11 3 -> /var/lib/sss/mc/passwd
1531231911.248787 ls[18975]: lrwx------. 1 root root 64 Jul 10 16:11 4 -> socket:[2845120]
1531231911.248787 ls[18975]: lr-x------. 1 root root 64 Jul 10 16:11 5 -> /var/lib/sss/mc/group
1531231911.248787 ls[18975]: lr-x------. 1 root root 64 Jul 10 16:11 6 -> /proc/18975/fd
1531231911.249933 systemd[1]: Started outputtest.service.
1531231911.262747 ls[18976]: total 0
1531231911.262747 ls[18976]: lrwx------. 1 root root 64 Jul 10 16:11 0 -> /dev/tty6
1531231911.262747 ls[18976]: lrwx------. 1 root root 64 Jul 10 16:11 1 -> socket:[2848262]
1531231911.262747 ls[18976]: lrwx------. 1 root root 64 Jul 10 16:11 2 -> socket:[2848262]
1531231911.262747 ls[18976]: lr-x------. 1 root root 64 Jul 10 16:11 3 -> /var/lib/sss/mc/passwd
1531231911.262747 ls[18976]: lrwx------. 1 root root 64 Jul 10 16:11 4 -> socket:[2850136]
1531231911.262747 ls[18976]: lr-x------. 1 root root 64 Jul 10 16:11 5 -> /var/lib/sss/mc/group
1531231911.262747 ls[18976]: lr-x------. 1 root root 64 Jul 10 16:11 6 -> /proc/18976/fd

It seems that this design has grown organically. Originally, StandardInput and StandardOutput were about log destination ("null", "tty", "journal"), so using the same target for ExecStartPre (and ExecStartPost, not sure, see #7069), made sense. Later on "socket" was added, which is somewhat surprising, but OK in most cases, as long as the process in ExecStartPre doesn't do anything with the descriptor. Finally, when file operations were added, e.g. file:, this stopped making much sense. For example, if a process has StandardOutput=file: and two ExecStart= lines, the file would be opened, possibly written to, then reopened with truncation, and possibly written to again:

# /etc/systemd/system/outputtest-file.service
[Service]
StandardOutput=file:/tmp/foo
ExecStart=echo aaa
ExecStart=echo bbb
Type=oneshot

(This produces bbb in /tmp/foo.)

Similarly with StandardInput=data, I don't think repeating the data is the intuitive thing.

I'm not sure what we should do with this. My idea would be to:

  • stop using the same input/output for ESPre= and ESPost= commands, if SI=data, SI=file, SO=file, SI=fd, SO=fd are used.
  • consider reusing the same fd (it would have to be stored in systemd for the duration of the service), in case Type=oneshot and multiple commands ExecStart= commands are specified. This would require the fd to be stored in pid1 for the duration of the service. But it would match what happens with SI=socket more closely.

So what happens, is that /tmp/test-exec-standardoutput-output is helpfully atomically replaced multiple times in that service, and the final test invocations get a nice new empty file too.

<para><option>append:<replaceable>path</replaceable></option> and <option>create:<replaceable>path
</replaceable></option> are similar to <option>file:<replaceable>path</replaceable></option> above,
but they open the file and append to it or replace it with a new, empty file instead of writing from the
beginning.</para>

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

This isn't bad, but I think we can be more precise:

<option>append:<replaceable>path</replaceable></option> and <option>create:<replaceable>path
</replaceable></option> are similar to <option>file:<replaceable>path</replaceable></option> above
<option>append:<replaceable>path</replaceable></option> opens the file <replaceable>path</replaceable>
in append mode.
<option>create:<replaceable>path</replaceable></option> atomically creates or replaces <replaceable>path</replaceable> with a new, empty file.
@@ -1786,6 +1787,11 @@ SystemCallErrorNumber=EPERM</programlisting>
specified path refers to an <constant>AF_UNIX</constant> socket in the file system, as in that case only a

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

Please also fix the "particular" / "particularly" typo in the line above.

This paragraph should be amended to say how "file:" opens the file.

if (o == EXEC_OUTPUT_FILE_CREATE) {
mode_t old_umask;
char *template, *output_dir;
output_dir = dirname_malloc(context->stdio_file[fileno]);

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

Please add an empty line between variable decls and the first executable line.

malloc can fail, so this needs an oom check. strjoin too. This memory is leaked, _cleanup_free_ should be added.

return -errno;
}
if (renameat(0, template, AT_FDCWD, context->stdio_file[fileno]) < 0) {
log_unit_error(unit, "failed to place output file");

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

Please include the file name in the error message, make it easier to understand if there's some failure what happened.

log_unit_error(unit, "failed to create temporary file");
return -errno;
}
if (renameat(0, template, AT_FDCWD, context->stdio_file[fileno]) < 0) {

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

-1 would be nicer, since 0 is a valid fd number. AT_FDCWD could/should be replaced too with -1, since this must be an absolute path. So just use rename ;)

read_full_file("/tmp/test-exec-standardoutput-output", &output, &out_size);
read_full_file("/tmp/test-exec-standardoutput-expected", &expected, &expected_size);
log_error("output(%u): %sx\nexpected(%u): %sx\n", (unsigned int)out_size, output, (unsigned int)expected_size, expected);
assert_se(streq(output, expected));

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

Using predictable names in tests is bad. I know we have some, but at least we shouldn't add new ones. Please remember to re-add PrivateTmp=yes to the service in the final version ;)

char *template, *output_dir;
output_dir = dirname_malloc(context->stdio_file[fileno]);
template = strjoin(output_dir, "/.systemd-output-XXXXXX");
old_umask = umask(0666 & ~context->umask);

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

Why ~ ? You should also use RUN_WITH_UMASK instead of manually resetting the mask. So this should look be just

                        RUN_WITH_UMASK(context->umask)
                                fd = mkostemp(template, flags);
Description=Test for StandardOutput=append:

[Service]
ExecStartPre=/bin/sh -c 'echo hello > /tmp/test-exec-standardoutput-output'

This comment has been minimized.

Copy link
@keszybz

keszybz Jul 10, 2018

Member

Just sh is enough. Also cat, cmp below.

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

Thanks for the help and thorough review, I don't think I would have gotten to the bottom of it alone.

consider reusing the same fd (it would have to be stored in systemd for the duration of the service), in case Type=oneshot and multiple commands ExecStart= commands are specified. This would require the fd to be stored in pid1 for the duration of the service. But it would match what happens with SI=socket more closely.

This sounds like the right approach to me, and I'd be happy to take a crack at it as a separate PR as it seems to be a bigger change. So how about this: let me drop support for create: in this PR, as append: and file: still works fine. I'll address the review comments, and then we can merge this. Then I'll open a new pull request to both make the above happen and add create:.

@zsol zsol force-pushed the zsol:append branch 2 times, most recently from 96c3652 to a12607b Jul 11, 2018

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Looks like the tests fail if I add PrivateTmp=true, but pass if I remove it. I wonder if there's a similar problem with setting up the private mount namespace per ES command?

@poettering

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

regarding the issue @keszybz ppinted out: i have the suspicion people would be quite surprised if StandardError=truncate:somefile wouldn't cause errrors from ExecStartPre= to be logged there, no?

I'd probably go a different way than what @keszybz suggested: let's do the "destructive" truncation operation only for the first ExecXYZ= operation we execute for each cycle. I.e. let's extend the ExecFlags= enum a bit, let's add EXEC_IS_FIRST or so. This mimics the existing EXEC_IS_CONTROL flag which tells the execution engine whether something is a "control" or "main" process we are executing.

When invoking exec_spawn() we should set EXEC_IS_FIRST based on some boolean we store in the Service structure, which is initialized on each service_start() call, and reset after each exec_spawn(). This new boolean would have to be serialized/deserialized too. Why maintain this this as separate boolean, instead of deriving the flag dynamically from what we are executing and the loaded Exec lines? Simply to make this more robust towards "systemctl daemon-reload", i.e. when unit configuration changes while we already started the unit.

@poettering
Copy link
Member

left a comment

looks pretty good, but probably shouldn't claim Fixes: in its current version. I am happy with merging this one before and independently of a future truncate: patch, but that means the issue should be left open until the second that is implemented too

<option>file:<replaceable>path</replaceable></option>, <option>socket</option> or
<option>fd:<replaceable>name</replaceable></option>.</para>
<option>file:<replaceable>path</replaceable></option>, <option>append:<replaceable>path</replaceable></option>,
, <option>socket</option> or<option>fd:<replaceable>name</replaceable></option>.</para>

This comment has been minimized.

Copy link
@poettering

poettering Jul 13, 2018

Member

one "," at the end of the line, and another one at the beginning of the next?

bool rw;
int fd;
int flags;

This comment has been minimized.

Copy link
@poettering

poettering Jul 13, 2018

Member

not that it matters, but why not merge this with the previous line...

@zsol zsol force-pushed the zsol:append branch from a12607b to 4ba1153 Jul 13, 2018

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Fair enough! Fixed the typos and changed the commit message to not close #8983 just reference it.

@zsol zsol changed the title Add support for opening files for appending or truncation Add support for opening files for appending Jul 13, 2018

@poettering

This comment has been minimized.

Copy link
Member

commented Jul 14, 2018

This breaks the ubuntu CI tests:

Child 9500 (cat) died (code=exited, status=0/SUCCESS)
exec-standardoutput-file.service: Child 9500 belongs to exec-standardoutput-file.service.
exec-standardoutput-file.service: Main process exited, code=exited, status=0/SUCCESS
exec-standardoutput-file.service: Running next main command for state start.
exec-standardoutput-file.service: Passing 0 fds to service
exec-standardoutput-file.service: About to execute: /usr/bin/cmp /tmp/test-exec-standardoutput-expected /tmp/test-exec-standardoutput-output
exec-standardoutput-file.service: Forked /usr/bin/cmp as 9501
Received SIGCHLD from PID 9501 (cmp).
Child 9501 (cmp) died (code=exited, status=1/FAILURE)
exec-standardoutput-file.service: Child 9501 belongs to exec-standardoutput-file.service.
exec-standardoutput-file.service: Main process exited, code=exited, status=1/FAILURE
exec-standardoutput-file.service: Failed with result 'exit-code'.
exec-standardoutput-file.service: Changed start -> failed
exec-standardoutput-file.service: Unit entered failed state.
Assertion 'service->main_exec_status.status == status_expected' failed at ../src/test/test-execute.c:59, function check(). Aborting.
Aborted (core dumped)
FAIL: test-execute (code: 134)
@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2018

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@poettering where did you see that log? I can't find it by clicking on the bionic-* checks's details

Also can't reproduce it locally :( Tests pass fine

@poettering

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@zsol hmm, so the CI was borked for unrelated reasons on your repush. See #9621.

The old CI showed the issue... Can you rebase on current git and repush? Hopefully the ubuntu CI gets far enough then, as #9621 has been fixed.

@zsol zsol force-pushed the zsol:append branch 2 times, most recently from d449385 to 2f5ab52 Jul 18, 2018

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Okay I realize what's happening here. ubuntu has a more minimal version of /bin/sh where the builtin echo doesn't recognize -e:

output(14): hello
o
hello
x
expected(14): -e hello
ello
x
Assertion 'streq(output, expected)' failed at ../src/test/test-execute.c:635, function test_exec_standardoutput(). Aborting.
@poettering

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Oh yeah, they have "dash", a major source of unnecessary bugs... I figure you can just use /bin/echo instead.

@zsol zsol force-pushed the zsol:append branch from 2f5ab52 to be3fa7c Jul 19, 2018

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Yes, that would've been good, too. I went with printf :)

Description=Test for StandardOutput=append:

[Service]
ExecStartPre=sh -c 'printf hello\\\\n > /tmp/test-exec-standardoutput-output'

This comment has been minimized.

Copy link
@filbranden

filbranden Jul 19, 2018

Member

You can also let systemd expand the newlines on itself... If you use a bare \n here it will do that. As long as the string looks quoted for the shell itself, you'll be fine.

See also an example here: https://github.com/systemd/systemd/blob/v239/test/TEST-04-JOURNAL/test.sh#L38

This should work I think:

ExecStartPre=sh -c 'printf "hello\n" > /tmp/test-exec-standardoutput-output'
ExecStartPre=sh -c 'printf "hello\nhello\n" > /tmp/test-exec-standardoutput-expected'

Or just use echo to avoid one of the \ns:

ExecStartPre=sh -c 'echo "hello" > /tmp/test-exec-standardoutput-output'
ExecStartPre=sh -c 'echo "hello\nhello" > /tmp/test-exec-standardoutput-expected'

(Another option is to use command grouping on the second line, to run two separate "echo" commands and still keep a single redirection):

ExecStartPre=sh -c 'echo "hello" > /tmp/test-exec-standardoutput-output'
ExecStartPre=sh -c '{ echo "hello"; echo "hello"; } > /tmp/test-exec-standardoutput-expected'

I think I like the middle one most... It's a bit annoying that the \n is expanded by systemd while the rest is shell (so there's two parsers at play and one needs to understand them really well to know what's really going on), but oh well that's systemd unit parsing for you :-)

Cheers,
Filipe

@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

These new CI errors look unrelated :(

@zsol zsol force-pushed the zsol:append branch from be3fa7c to 566b7d2 Jul 20, 2018

@poettering

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

CI passed now. Let's merge!

@poettering poettering merged commit f606cd1 into systemd:master Jul 20, 2018

7 checks passed

Fedora Rawhide CI x86_64 rpm build [succeeded]
Details
LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No alert changes
Details
bionic-amd64 autopkgtest finished (success)
Details
bionic-i386 autopkgtest finished (success)
Details
bionic-s390x autopkgtest finished (success)
Details
semaphoreci The build passed on Semaphore.
Details
@zsol

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

whew, what a journey. Thanks everyone for all the help. I'll be opening the next one about truncate: soon

@zsol zsol deleted the zsol:append branch Jul 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.