I am really not a fan of full code lines passed to macros as parameters.
Let's get rid of the 3rd parameter of FOREACH_OPTION() hence:
1. Let's return errors just as a regular value (though a negative one),
that can be handled via a OPTION_ERROR case statement for the switch.
This normalizes handling of the error, just like any other event
returned by the option parser.
2. In order to avoid exploding the amount of boilerplate in each use
(that just propagates the error on OPTION_ERROR), let's then
introduce an explicit FOREACH_OPTION_OR_RETURN(), that returns from
the calling function on its own (and makes that clear in the name).
Together this cleans up, normalizes the logic and shortens the code.
It was requested to make the 'c', 'opt', and 'arg' params the same, i.e.
defined through the FOREACH_OPTION macro. But we can't do that easily,
because 'c' was defined in the for loop definition, and we can only
define variables of the same type in that way. Also, in some cases we
need only 'c', in other cases with need 'c' and 'arg, in some cases 'c'
and 'opt', and in other cases all three. We'd need to either
conditionalize or mark those variables with _unused_ to deal with
compiler warnings. But a different approach works quite nicely: add
state.opt and state.arg to show the current option and it's argument.
(The short names are picked on purpose to reduce verbosity since those
are used a lot.)
A few --help strings are changed to the common option variants.
The optional args for --unit, --user-unit, --xattr, --cgroup-id
are shown in synopsis.
A define is defined for the default of --depth to make things
clearer.
Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
Multiple callers of cg_get_keyed_attribute() follow the same pattern of
reading a single keyed attribute and then parsing it as uint64_t with
safe_atou64(). Add a helper that combines both steps.
Convert all existing single-key + uint64 call sites in cgtop, cgroup.c,
and oomd-util.c to use the new helper.
The fix in 8f1751a111 made me wonder if we could automatically detect
when pointers are accessed but when this might not be safe. Systemd
is already using a lot of `assert(dst)` and this change now forces
us to use them.
So this commit (ab)uses coccinelle to flag any pointer parameter
dereference not preceded by assert(param), ASSERT_PTR(param), or an
explicit NULL check. It adds integration into meson as a new "coccinelle"
test suite (just like clang-tidy) and is run in CI. The check is not
perfect but seems a reasonable heuristic.
For this RFC commit it is scoped to a subset, it excludes 25 dirs right
now and includes around 100. About 300 warnings left. Busywork that I am
happy to do if there is agreement that it is worth it.
With this in place we would have caught the bug from 8f1751a111 in CI:
```
FAIL: check-pointer-deref.cocci found issues in systemd/src/boot:
diff -u -p systemd/src/boot/measure.c /tmp/nothing/measure.c
--- systemd/src/boot/measure.c
+++ /tmp/nothing/measure.c
@@ -312,7 +312,6 @@ EFI_STATUS tpm_log_tagged_event(
if (err != EFI_SUCCESS)
return err;
- *ret_measured = true;
return EFI_SUCCESS;
}
```
This also adds a new POINTER_MAY_BE_NULL() for the cases when the
called function will do the NULL check (like `iovec_is_set()`).
When we say '-n --iterations=N' in --help, this means that the program
can be invoked with '-n N' or '--iterations=N' or '--iterations N'.
(The short option is specified without the argument.)
Here we tried to use '-p --order=path' to mean that the program can be
invoked with '-p' or '--order=path', but that is incompatible with the
established convention.
Also, indicate that the arg to --cpu is optional.
This commit cleans up the includes for all the small tools across
the tree.
A few cases of returning EXIT_SUCCESS are replaced with returning
0 to avoid including <stdlib.h>.
Split out of #37344.
Now that the necessary functions from log.h have been moved to macro.h,
we can stop including log.h in macro.h. This requires modifying source
files all over the tree to include log.h instead.
1. Make the message a bit more visible, by adding ANSI color. This
matters in particular during boot, where the message otherwise might
be overprinted by other output
2. Let's turn off terminal echo so that whatever key is entered is not
made visible on screen, and we can handle newline and other keys
reasonably uniformly.
We use usec_t for storing time value, which is 64bit.
However, usleep() takes useconds_t that is (typically?) 32bit.
Also, usleep() may only support the range [0, 1000000].
This introduce usleep_safe() which takes usec_t.
In the documentation we usually spell the concept "control group".
Internally in code we usually call it "cgroup" or "CGroup". In systemctl output we
called the field "CGroup" so far, i.e. a capitalized version of the
internal name. This is of course very unsystematic. Let's clean this up
a bit: let's now say:
* in docs, continue to spell it out "control groups"
* in brief output call it "CGroup"
* internally call it "cgroup" or "CGroup"
Fixes: #14429
--recursive=no will overwrite possible -P or -k option hence making the
recursive disabling impossible.
Check what counting types the system supports (encoded in the ordering
of our enum) of and pick whatever user requests but is also supported.
Fixes: #25248
Unfortunately, hex output can only be produced with unsigned types. Some
cases can be fixed by producing the correct type, but a few simply have
to be cast. At least casting makes it explicit.
Same idea as 03677889f0.
No functional change intended. The type of the iterator is generally changed to
be 'const char*' instead of 'char*'. Despite the type commonly used, modifying
the string was not allowed.
I adjusted the naming of some short variables for clarity and reduced the scope
of some variable declarations in code that was being touched anyway.
According to Coverity, 194 ouf of 227 times we check for snprintf return code.
Voidify the rest.
CID#1461512
CID#1461513
CID#1461514
CID#1461515
CID#1461516
CID#1461518
CID#1461519
CID#1461520
CID#1461522
In general we almost never hit those asserts in production code, so users see
them very rarely, if ever. But either way, we just need something that users
can pass to the developers.
We have quite a few of those asserts, and some have fairly nice messages, but
many are like "WTF?" or "???" or "unexpected something". The error that is
printed includes the file location, and function name. In almost all functions
there's at most one assert, so the function name alone is enough to identify
the failure for a developer. So we don't get much extra from the message, and
we might just as well drop them.
Dropping them makes our code a tiny bit smaller, and most importantly, improves
development experience by making it easy to insert such an assert in the code
without thinking how to phrase the argument.
This nicely covers the case when optarg is optional. The same parser can be
used when the option string passed to getopt_long() requires a parameter and
when it doesn't.
The error messages are made consistent.
Also fixes a log error c&p in --crash-reboot message.
I think this formatting was originally used because it simplified
adding new options to the help messages. However, these days, most
tools their help message end with "\nSee the %s for details.\n" so
the final line almost never has to be edited which eliminates the
benefit of the custom formatting used for printf() help messages.
Let's make things more consistent and use the same formatting for
printf() help messages that we use everywhere else.
Prompted by https://github.com/systemd/systemd/pull/18355#discussion_r567241580
Luca Boccassi
Lennart Poettering
Zbigniew Jędrzejewski-Szmek
Yaping Li
Michael Vogt
Zbigniew Jędrzejewski-Szmek
Yu Watanabe
David Tardon
Daan De Meyer
Mike Yuan
Lennart Poettering
Timo Rothenpieler
Luca Boccassi
Joerg Behrmann
Michal Koutný
Jan Janssen
Luca Boccassi