fstab-generator: pass k instead of r to bus_error_message() so the
fallback error string reflects the actual bus call failure, not the
accumulated result that was reset to 0 earlier.
networkd-ndisc: return -ENOMEM when newdup() fails, since r is 0 at
that point and the OOM would otherwise be reported as success.
storagetm: add missing NULL check after strndup() for attr_model,
matching the pattern already used for attr_firmware and attr_serial.
Signed-off-by: dongshengyuan <dongshengyuan@uniontech.com>
Co-developed-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.)
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()`).
get_block_device_harder_fd() currently only traverses one level of
device mapper stacking when looking for the underlying block device.
This causes issues with nested DM setups like dm-crypt on top of
dm-integrity, where we don't traverse enough to get the actual physical
device.
Fix this by iterating through all DM layers until we reach a device with
no underlying device. And while we're at it also make cgroups use the
same logic.
Fixes: #40419
device_track_back() only checks a single DM layer when resolving the
originating device. Use
block_device_get_originating(..., /* recursive= */ true) to follow
stacked layers.
Originating device lookups currently use separate entry points and
callers must decide between the single-step helper and manual looping.
Add a recursive flag to block_device_get_originating() and
block_get_originating(), and update callers to pass an explicit value.
udev's block device locking protocol has one pitfall not even the
example in the documentation got right so far (even though this is
explained in all detail above): udev's rescanning is only triggered when
an fd that is opened for writing is closed. This means that if a
separate locking fd is opened on a block device – one that is maintained
independently of the fd actually used for writing – it must be opened for
writing too, so that closing the lock definitely triggers a rescan. This
matters in cases where the lock fd is kept for longer than the fd used
for writing to disk. (Because otherwise udev might get the
IN_CLOSE_WRITE event, but when it tries to rescan will find the device
locked, and never retry because no IN_CLOSE_WRITE is triggred anymore.)
Let's fix that across the codebase, at 4 places:
1. in makefs (a lock fd is kept, and mkfs then invoked as child, which
uses a different fd, and the lock fd is closed only once the child
died)
2. in udevadm lock (embarassing!): which is intended to be used to wrap tools
that modify disk contents, very similar to the makefs case. The lock
is also kept until after the tool exited.
3. In storagetm: the kernel nvme-tcp layer writes to the device
directly, we just keep a lock fd.
4. the example in BLOCK_DEVICE_LOCKING.md
git grep -l 'Failed to open /'|xargs sed -r -i 's|"Failed to open (/[^ ]+): %m"|"Failed to open %s: %m", "\1"|g'
git grep -l $'Failed to open \'/'|xargs sed -r -i $'s|"Failed to open \'(/[^ ]+)\': %m"|"Failed to open %s: %m", "\\1"|g'
git grep -l "Failed to open /"|xargs sed -r -i $'s|"Failed to open (/[^ ]+), ignoring: %m"|"Failed to open %s, ignoring: %m", "\\1"|g'
+ some manual fixups.
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.
We have two typical styles of 'ret' param assignment + retval:
1) < 0 on actual error, 0 on nothing (ret == NULL), > 0 on something
2) recognizable errno on nothing, < 0 on other errors, >= 0 on something
but never use both at the same time.
Currently, NOTIFY_READY from daemon-util.h conflicts with NOTIFY_READY
from NotifyState from service.h so let's rename the constants to avoid
the conflict.
Admittedly, some of our glyphs _are_ special, e.g. "O=" for SPECIAL_GLYPH_TOUCH ;)
But we don't need this in the name. The very long names make some invocations
very wordy, e.g. special_glyph(SPECIAL_GLYPH_SLIGHTLY_UNHAPPY_SMILEY).
Also, I want to add GLYPH_SPACE, which is not special at all.
To make the generated IDs equivalent when
- sd_device object is not provided,
- sd_device object is provided, but it does not have ID_SERIAL.
Follow-up for abc19a6ffa.
This also fixes missing voidification.
Fixes CID#1524253.
don't let the devices to be announced just as model "Linux". Let's instead
propagate the underlying block device's model. Also do something
reasonably smart for the serial and firmware version fields.
This implements a "storage target mode", similar to what MacOS provides
since a long time as "Target Disk Mode":
https://en.wikipedia.org/wiki/Target_Disk_Mode
This implementation is relatively simple:
1. a new generic target "storage-target-mode.target" is added, which
when booted into defines the target mode.
2. a small tool and service "systemd-storagetm.service" is added which
exposes a specific device or all devices as NVMe-TCP devices over the
network. NVMe-TCP appears to be hot shit right now how to expose
block devices over the network. And it's really simple to set up via
configs, hence our code is relatively short and neat.
The idea is that systemd-storagetm.target can be extended sooner or
later, for example to expose block devices also as USB mass storage
devices and similar, in case the system has "dual mode" USB controller
that can also work as device, not just as host. (And people could also
plug in sharing as NBD, iSCSI, whatever they want.)
How to use this? Boot into your system with a kernel cmdline of
"rd.systemd.unit=storage-target-mode.target ip=link-local", and you'll see on
screen the precise "nvme connect" command line to make the relevant
block devices available locally on some other machine. This all requires
that the target mode stuff is included in the initrd of course. And the
system will the stay in the initrd forever.
Why bother? Primarily three use-cases:
1. Debug a broken system: with very few dependencies during boot get
access to the raw block device of a broken machine.
2. Migrate from system to another system, by dd'ing the old to the new
directly.
3. Installing an OS remotely on some device (for example via Thunderbolt
networking)
(And there might be more, for example the ability to boot from a
laptop's disk on another system)
Limitations:
1. There's no authentication/encryption. Hence: use this on local links
only.
2. NVMe target mode on Linux supports r/w operation only. Ideally, we'd
have a read-only mode, for security reasons, and default to it.
Future love:
1. We should have another mode, where we simply expose the homed LUKS
home dirs like that.
2. Some lightweight hookup with plymouth, to display a (shortened)
version of the info we write to the console.
To test all this, just run:
mkosi --kernel-command-line-extra="rd.systemd.unit=storage-target-mode.target" qemu
dongshengyuan
Lennart Poettering
Zbigniew Jędrzejewski-Szmek
Michael Vogt
Yu Watanabe
Mike Yuan
Chris Down
Lennart Poettering
Zbigniew Jędrzejewski-Szmek
Jelle van der Waa
Daan De Meyer
Luca Boccassi
Mike Gilbert