Let's nudge people towards the use of an anonymous buffer like we
do internally.
"errno" → "errnum", to match the man page for strerror, and also to avoid
confusion with the global variable. In general, I think that errno is a
terrible interface and we shouldn't encourage people to use it. Those functions
use errno-style error numbers, which are a different thing.
The need to set errno is very very ugly, but at least it is thread-safe and
works correctly. Using strerror() is likely to be wrong, so let's not recommend
that. People who do a lot of logging would provide use some wrapper that sets
errno like we do, so nudge people towards %m.
I tested that all the separate .c files compile cleanly.
sd-bus: make bus_error_message() a thread-safe macro
strerror_r() is used instead of strerror(). The usual trick is employed: we
allocate a buffer that lives until the end of the surrounding block to provide
the scratch space. This change is particularly important forn sd-bus and the
pam modules, which may be called from threaded code.
I checked the codebase, and we only use bus_error_message() in log statements,
so the returned pointer is not used beyond its valid lifetime.
shared/journal-importer: use %m instead of strerror()
Here SYNTHETIC_ERRNO() was used based on the general rule that logging
functions should do that when the error value is generated at the call
site. But here we're really propagating a memory allocation error, which
wasn't reported using errno, but the meaning is the same. And it's better
to bend the rule a bit like this than to use strerror().
core,logind,systemctl,journald: replace calls to strerror() with setting errno + %m
strerror() is not thread safe and calling it just isn't worth the effort
required to justify why it would be safe in those cases. It's easier to just
use %m which is thread-safe out of the box. I don't think that any of the
changes in the patch cause any functional difference. This is just about
getting rid of calls to strerror() in general.
When we print an error message and fail to format the string, using something
like "(null)" is good enough. This is very very unlikely to happen anyway.
Error handling in acquire_user_record() was checking the wrong
condition (PAM errors are always >= 0, so r < 0 cannot match).
Apart from the fix for error handling, no change in behaviour is intended.
I did some minor adjustements to formatting and added _cleanup_ in one more
place.
This is a primitive helper that wraps calls to pam_syslog() replacing
@PAMERR@ with pam_strerror() output in the format string. This allows for
a bunch of boilerplate to be removed.
@PAMERR@ is only supported at the end of the string. Similarly to %m,
realistically that's the only place where it is useful.
Note that unlike in logging functions in log.[ch], here the error value is
only used for the message and is not saved anywhere, so we don't need to
care about SYNTHETIC_ERRNO.
shared/pam-util: add pam_syslog_errno() wrapper that sets errno
So far our pam code was using strerror_safe(). But that's not a good approach,
because strerror_safe() is not thread-safe, and the pam code is "library code"
that should be thread-safe. In fact, the whole effort to use strerror() is
unnecessary, because pam_syslog() is documented to support %m. The
implementation in linux-pam simply uses vasprintf(). If we use %m too, we get
rid of the issue. The wrapper sets errno temporarily from the argument.
Apparently some PAM consumers run multiple PAM stacks in threads, so we should
avoid non-thread-safe code.
The new helper returns PAM_BUF_ERR for ENOMEM, and PAM_SERVICE_ERR in other
cases. This may change the returned code in some cases, but I think a) it
doesn't matter much, b) it's probably for the better. E.g. we might now return
PAM_SERVICE_ERR if the dbus message is borked, and PAM_SERVICE_ERR seems
appropriate.
basic/log: include the log syntax callback in the errno protection block
In general, log_syntax_internal() must keep errno unchanged. But the
call to log_syntax_callback() was added outside of the block protected
by PROTECT_ERRNO.
Thomas Hebb [Thu, 29 Sep 2022 06:40:35 +0000 (23:40 -0700)]
network: don't forget old RAs when a new one arrives
IPv6 Neighbor Discovery lets us autoconfigure a link's IPv6 addresses,
routes, DNS servers, and DNS search domains by listening for Router
Advertisement (RA) packets broadcast by one or more routers on the link.
Each RA can contain zero or more "options," each describing one piece of
configuration (e.g. a single route).
Currently, when we receive an RA from a router, we delete any addresses,
routes, etc. that originated from that router's previous RAs unless
they're also present as options in the new RA.
That behavior is a violation of RFC 4861[1]. In Section 9, the RFC
states that
Senders MAY send a subset of options in different packets. ... Thus,
a receiver MUST NOT associate any action with the absence of an
option in a particular packet. This protocol specifies that
receivers should only act on the expiration of timers and on the
information that is received in the packets.
Several other passages in the RFC reiterate this. Section 6.2.3:
A router MAY choose not to include some or all options when sending
unsolicited Router Advertisements.
Section 6.3.4:
Hosts accept the union of all received information; the receipt of a
Router Advertisement MUST NOT invalidate all information received in
a previous advertisement or from another source.
At least one consumer router in production today, the Google Nest Wifi,
often sends RAs that omit its global IPv6 prefix. When current versions
of systemd-networkd receive those RAs, they immediately delete the
interface's global IPv6 address, which breaks IPv6 connectivity.
Fix the issue by removing the invalidation logic entirely. It's not
needed at all, since we already invalidate addresses, routes, and DNS
configuration when the interface goes down or their lifetimes expire.
This fix does have the side effect of preventing changes to the .network
file (e.g. denylisted prefixes, whether to add routes from RAs) from
taking effect as soon as a new RA arrives. Instead, a full interface
reconfiguration is needed. But triggering those changes on RA receipt
was already rather arbitrary and out of the administrator's control, so
I think this change is fine.
commit 69203fba700e ("network: ndisc: remove old addresses and routes
after at least one SLAAC address becomes ready") introduced this
behavior. commit 50550722e3ba fixed it partially, by preventing one
router's RAs from invalidating another router's configuration.
[1] https://www.rfc-editor.org/rfc/rfc4861
Fixes: 69203fba700e ("network: ndisc: remove old addresses and routes after at least one SLAAC address becomes ready")
The iscsi-init.service calls `sh` which might, in certain circumstances,
pull in instrumented systemd NSS modules causing `sh` to fail. Let's mitigate
this by pulling in an env file crafted by `create_asan_wrapper()` that
(among others) pre-loads ASan's DSO.
test: expand the expression in `cleanup_initdir()`
Otherwise we might unexpectedly return 1 if the `get_bool` call fails.
If the `get_bool` part in `get_bool "$TEST_SETUP_CLEANUP_ROOTDIR" && _umount_dir "${initdir:?}"`
fails, the whole expression will short-circuit evaluate to 1, and since it's
the last expression in the function it's also it's return value, which doesn't
reflect the original intent of the expression:
ci: pin stefanbuck/github-issue-parser to a tagged release
Since [0] got resolved ([1]) we can finally pin the action to a tagged
release (v2.0.4 ATTOW) and let Dependabot to do its job by updating it
to the latest tagged release when it becomes available.
manager: fix/change evaluation of ConditionFirstBoot
The code to evaluate the kernel command line option was busted because it
was doing 'return b == !!r' at a point where 'r > 0'. Thus we'd return "true"
in both cases:
We only use 'ConditionFirstBoot=true' in units, so this wasn't noticed.
But I think the logic is broken in general: the condition should evaluate as
true only during initial boot. If we rerun the units at later points, we should
not consider ConditionFirstBoot to be true.
Also, the first boot logic is also used in pid1 itself. AFAICT, for two
things: in first boot machine-id is initialized transiently (this allows
first-boot operations to be restarted if boot fails), and preset-all is
executed. But this logic was different and separate from the logic to
evaluate ConditionFirstBoot. The distinction is abolished, and the operations
in pid1 now use the same logic as ConditionFirstBoot, which means that the
kernel command line option is checked, and condition_test_first_boot()
just tests whether pid1 thinks we're in first boot.
This makes things easier to grok for the user: there's just one condition for
"first boot" and it applies to both pid1 and units.
$ SYSTEMD_LOG_LEVEL=debug build/systemd-firstboot --prompt-root-password
Found container virtualization systemd-nspawn.
Found /etc/locale.conf, assuming locale information has been configured.
Failed to read credential firstboot.keymap, ignoring: No such device or address
Prompting for keymap was not requested.
Found /etc/localtime, assuming timezone has been configured.
Prompting for hostname was not requested.
Found /etc/machine-id, assuming machine-id has been configured.
Found /etc/passwd and /etc/shadow, assuming root account has been initialized.
Creation of /etc/kernel/cmdline was not requested, skipping.
Christian Hesse [Fri, 30 Sep 2022 08:26:43 +0000 (10:26 +0200)]
systemctl: color ignored exit status in yellow, not red
If the executable path is prefixed with "-", an exit code of the command
normally considered a failure (i.e. non-zero exit status or abnormal exit
due to signal) is recorded, but has no further effect and is considered
equivalent to success.
Let's honor this with `systemctl status`, and color ignored exit status
in yellow, not red.
Jonas Kümmerlin [Thu, 29 Sep 2022 16:51:03 +0000 (18:51 +0200)]
generator: skip fsck if fsck command is missing
This is useful for systems which don't have any fsck.
We already skip emitting the fsck dependency when the fsck.$fstype helper
is missing, but fstab-generator doesn't necessarily know the fstype when
handling the root= parameter.
Previously, systemd-fsck was started for these mounts and then exited
immediately because it couldn't find the fsck.$fstype helper.
Allows to skip check that ensures units must not be running.
I have a use case that would use reattach, except the orchestrator
is using a non-standard versioning scheme, so image matching cannot
work. As a workaround, need to be able to detach and then attach
manually, without stopping the units to avoid extended downtimes
and loss of FD store.
1. Use Type=notify to wait until "resolvectl monitor" successfully
installed its monitor, so that we know that queries enqueued later
will definitely be seen.
2. Use "grep -m1" to watch "journalctl -f" output to wait precisely for
the RR data we want to see, and immediately exit.
This shortens code quite a bit, and should make it more robust.
resolved: add generic formatters for RRs into JSON
For now we can use it to send broken-down records of JSON via the
monitor logic, but one day we can certainly reuse for dumping the
caches, or to implement a ResolveRecord() varlink call one day.
The socket is only accessible to privileged clients anyway, no need to
add another (user unfriendly) restriction via opt-in setting. let's just
allow this for privileged clients, mirroring "busctl monitor", or
"tcpdump" and similar, which all just work if you have privs.
(This does not break API, since we never did a release witht the
"Monitor" dbus property or config setting in place, i.e. with cb456374e096f0ebe9b70d7ddd98e16a4be24ee6)
sd-event: add helper for exiting event loop on SIGTERM/SIGINT
In many (most?) of our event loops we want to exit once SIGTERM/SIGINT
is seen. Add a common helper for that, that does the right things in a
single call.
sd-event: if signal nr has high bit set sd_event_add_signal() auto-block it via sigprocmask()
So far we expected callers to block the signals manually. Which is
usually a good idea, since they should do that before forking off
threads and similar. But let's add a mode where we automatically block
it for the caller, to simplify things.
test: wait until the unit finishes before checking the log
Otherwise we might read an incomplete log and fail:
```
test_added_after (__main__.ExecutionResumeTest) ... FAIL
test_added_before (__main__.ExecutionResumeTest) ... ok
test_interleaved (__main__.ExecutionResumeTest) ... ok
test_issue_6533 (__main__.ExecutionResumeTest) ... ok
test_no_change (__main__.ExecutionResumeTest) ... ok
test_removal (__main__.ExecutionResumeTest) ... ok
test_swapped (__main__.ExecutionResumeTest) ... ok
======================================================================
FAIL: test_added_after (__main__.ExecutionResumeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/build/./test/test-exec-deserialization.py", line 152, in test_added_after
self.check_output(expected_output)
File "/build/./test/test-exec-deserialization.py", line 107, in check_output
self.assertEqual(output, expected_output)
AssertionError: 'foo\n' != 'foo\nbar\n'
foo
+ bar
----------------------------------------------------------------------
Ran 7 tests in 27.470s
```
Adam Williamson [Thu, 29 Sep 2022 19:58:03 +0000 (12:58 -0700)]
kbd-model-map: correct variants for cz-qwerty to include comma
As explained by @poncovka , the 'xvariant' string should contain
the same number of comma-separated elements as 'xlayout'. When
we have two layouts we need two items in xvariant, in this case
one of them is empty.
See https://github.com/rhinstaller/anaconda/pull/4355#pullrequestreview-1119913870
for @poncovka's full explanation.
Signed-off-by: Adam Williamson <awilliam@redhat.com>
Add special keyboard combos for Thinkpad P1 Gen 3 (#24862)
* Add special keyboard combos for Thinkpad P1 Gen 3
These are based on the key codes I've found with evtest. See issue
https://github.com/systemd/systemd/issues/24814 for more details.
I'm not entirely sure what some of these keys are supposed to do,
notably Fn+RShift; this doesn't seem to do anything in Windows on
my machine. Binding them to prog# makes them available to desktop
managers' key bindings at least, in case someone wishes to make
use of this extra keybind possibility.
udevadm: do not try to find device unit when a path like string is provided
Otherwise, we provide misleading error message.
Before:
---
$ udevadm info /sys/class/foo
Bad argument "/sys/class/foo", expected an absolute path in /dev/ or /sys/ or a unit name: Invalid argument
---
After:
---
$ udevadm info /sys/class/foo
Unknown device "/sys/class/foo": No such device
---
docs/CONTRIBUTING: explain various labels and add link to "reviewable" PRs
The linked filter gives an up-to-date list of pull requests that need review.
(Yes, there's too many.) We used to set 'needs-review' label, but that is
not available to non-members, and also every pull requests which is not labeled
'reviewed/needs-rework'/'ci-fails/needs-rework'/'needs-rebase' can and should
be reviewed.
If this is merged, I'll drop the 'needs-review' label.
test: wrap `ls` and `stat` to make it work w/ sanitizers in specific cases
When `/etc/nsswitch.conf` uses `systemd` together with `[SUCCESS=merge]`,
`ls -l` will pull in `libnss_systemd` causing `SIGABRT`, as `ls` is not
instrumented (by default):
```
-bash-5.1# strace -f -e %file ls -l /dev
execve("/usr/bin/ls", ["ls", "-l", "/dev"], 0x7ffc3bb211c8 /* 24 vars*/) = 0
...
openat(AT_FDCWD, "/etc/passwd", O_RDONLY|O_CLOEXEC) = 3
newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=1896, ...}, AT_EMPTY_PATH) = 0
newfstatat(AT_FDCWD, "/etc/nsswitch.conf", {st_mode=S_IFREG|0644, st_size=359, ...}, 0) = 0
openat(AT_FDCWD, "/etc/group", O_RDONLY|O_CLOEXEC) = 3
newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=965, ...}, AT_EMPTY_PATH) = 0
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=10779, ...}, AT_EMPTY_PATH) = 0
openat(AT_FDCWD, "/usr/lib/libnss_systemd.so.2", O_RDONLY|O_CLOEXEC) = 3
newfstatat(3, "", {st_mode=S_IFREG|0755, st_size=16195176, ...}, AT_EMPTY_PATH) = 0
openat(AT_FDCWD, "/usr/lib/libasan.so.8", O_RDONLY|O_CLOEXEC) = 3
...
readlink("/proc/self/exe", "/usr/bin/ls", 4096) = 11
open("/proc/self/cmdline", O_RDONLY) = 3
open("/proc/self/environ", O_RDONLY) = 3
==620==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.
--- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=620, si_uid=0} ---
+++ killed by SIGABRT (core dumped) +++
Aborted (core dumped)
```
This also happens with `stat`. Let's add both `ls` and `stat` to the "wrap list"
to work around this.
projects / thirdparty / systemd.git / log
