util-lib: rework get_process_cmdline() #3529
Conversation
| assert_se(streq(line, "[testa]")); | ||
| line = mfree(line); | ||
|
|
||
| assert_se(write(fd, "\0\0\0\0\0\0\0\0\0", 10) == 10); |
There was a problem hiding this comment.
idk if this is just placeholder, but cmdline really could be just NULs.
max_length == 0 -> len would be 0 but line would still be allocated and returned
max_length > 1 -> k would be still equal to r and result in ENOENT
There was a problem hiding this comment.
you mean if comm_fallback is false, then an entirely zero (but not empty) cmdline would result in the empty string being returned, but it should return -ENOENT?
Are you sure it should?
or maybe i am not grokking what you are saying?
There was a problem hiding this comment.
Yes, correct. I was just running dummy setproctitle process that zeroes argv and test-process-util (pid) fails instantly, because of comm_fallback=false at the start of testcases. But there is no empty string result
PID3032 comm: 'setproctitle'
PID3032 cmdline: '913 27745 34816 4913 4194304 95 0 0 0 0 0 0 0 20 0 1 0 314104177 8953856a
Assertion 'get_process_cmdline(pid, 8, false, &d) >= 0' failed at src/test/test-process-util.c:65, function test_get_process_comm(). Aborting.
Aborted
There was a problem hiding this comment.
I think we should really treat an all-zero cmdline also as a reason to fall back to the comm field. Hence i think the current behaviour is actually right. That said, we should test this explicitly, hence I will post an updated patch shortly, that explicitly checks for this.
This reworks get_process_cmdline() quite substantially, fixing the following: - Fixes: systemd@a4e3bf4#r66837630 - The passed max_length is also applied to the "comm" name, if comm_fallback is set. - The right thing happens if max_length == 1 is specified - when the cmdline "foobar" is abbreviated to 6 characters the result is not "foobar" instead of "foo...". - trailing whitespace are removed before the ... suffix is appended. The 7 character abbreviation of "foo barz" is hence "foo..." instead of "foo ...". - leading whitespace are suppressed from the cmdline - a comprehensive test case is added
poettering
force-pushed
the
get-process-cmdline
branch
from
2d6214b
to
d7193a5
Compare
|
OK, I force pushed a new version that leaves the behaviour as before, but extends the test to verify this explicitly. This means we will fallback to comm whenever the cmdline is entirely absent or contains only zeroes, and is thus empty. I am pretty sure this is the right behaviour to follow here, as this call is primarily for presentational purposes and as such we should try to return something more useful than the empty string if we can. I also added a longer comment to the function describing its behaviour, in particular some corner cases. |
$ sudo make check TESTS=test-process-util
[...]
FAIL: test-process-util
[...]
$ cat ./test-process-util.log
PID1 comm: 'systemd'
PID1 cmdline: '/usr/lib/systemd/systemd --system --deserialize 17'
PID1 cmdline truncated to 8: '/usr...0'
PID1 cmdline truncated to 1: ''
PID1 PPID: 0
PID1 exe: '/usr/lib/systemd/systemd'
PID1 UID: 0
PID1 GID: 0
PID1 strlen(environ): 0
PID1 $PATH: 'n/a'
PID29210 comm: 'test-process-ut'
PID29210 cmdline: './test-process-util'
PID29210 cmdline truncated to 8: './te...'
PID29210 cmdline truncated to 1: ''
PID29210 PPID: 29209
PID29210 exe: '/home/vagrant/systemd/test-process-util'
PID29210 UID: 0
PID29210 GID: 0
PID29210 strlen(environ): 2199
PID29210 $PATH: '/sbin:/bin:/usr/sbin:/usr/bin'
Assertion 'get_process_cmdline(getpid(), 0, false, &line) == -ENOENT' failed at src/test/test-process-util.c:229, function test_get_process_cmdline_harder(). Aborting.
Assertion 'si.si_code == CLD_EXITED' failed at src/test/test-process-util.c:173, function test_get_process_cmdline_harder(). Aborting.
FAIL test-process-util (exit status: 134)
...
0 "[t...]"
test_get_process_cmdline_harder+0x73e [test-process-util]
main+0x85 [test-process-util]
__libc_start_main+0xf0 [libc-2.22.so]
_start+0x29 [test-process-util]
--
0 "[testa]"
test_get_process_cmdline_harder+0x7e0 [test-process-util]
main+0x85 [test-process-util]
__libc_start_main+0xf0 [libc-2.22.so]
_start+0x29 [test-process-util]
--
0 "\f$\255\373"
test_get_process_cmdline_harder+0x8c9 [test-process-util]
main+0x85 [test-process-util]
__libc_start_main+0xf0 [libc-2.22.so]
_start+0x29 [test-process-util]
ABRT |
|
Do we need the |
This reworks get_process_cmdline() quite substantially, fixing the following:
a4e3bf4#r66837630
set.
"foobar" instead of "foo...".
character abbreviation of "foo barz" is hence "foo..." instead of "foo ...".