sysupdate: refuse reboot/pending logic when --component= is used#42578
Conversation
github-actions
Bot
added
documentation
tests
sysupdate
please-review
Claude review of PR #42578 (19e40a6)The change is correct and complete: every path that compares the newest installed version against the host Suggestions
Nits
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@bluca I have addressed both points from the claude-review pass in the latest push: moved the |
1211efc to
548d546
Compare
poettering
added
good-to-merge/after-next-release
and removed
please-review
|
lgtm |
The `pending` and `reboot` verbs, as well as the `--reboot` switch, compare the newest installed version against the booted OS version (IMAGE_VERSION= from os-release). When a component is selected via --component=, this compares the component's version against the unrelated host OS version, which by design live in separate version spaces. The result is arbitrary reboot decisions: depending on the relative version strings sysupdate would either always or never reboot. Refuse the combination with a clear error instead of silently performing a bogus comparison. Correctly tracking a per-component booted version is left as a future feature. Fixes: systemd#42330
Move the --reboot/--component= rejection into parse_argv() alongside the other cross-option checks, and tighten TEST-72 to assert the specific guard message rather than merely a non-zero exit.
548d546 to
19e40a6
Compare
| (! "$SYSUPDATE" --component=some-component pending) |& grep -F 'may not be combined' >/dev/null | ||
| (! "$SYSUPDATE" --component=some-component reboot) |& grep -F 'may not be combined' >/dev/null | ||
| (! "$SYSUPDATE" --component=some-component update --reboot) |& grep -F 'may not be combined' >/dev/null |
There was a problem hiding this comment.
Claude: suggestion: grep -F 'may not be combined' is loose enough that it doesn't confirm the component guard is what fired. That substring appears in several guard messages in sysupdate.c — the --root=/--image= guards in verb_pending_or_reboot() and parse_argv(), the --definitions= guard, and the two new --component= guards. The three invocations here pass only --component=, so today only the intended guards can match and the assertion is correct. But it is fragile: a future change that made one of these commands hit a different 'may not be combined' check first (e.g. a default --root=, or reordered cross-option checks) would keep the test green while no longer exercising the component behaviour it claims to. Grepping for a distinctive part of the message (e.g. --component= together with the booted-OS-version phrasing) would pin each assertion to the actual guard.
3 participants
Fixes #42330
The
pendingandrebootverbs, and the--rebootswitch, decide whether to reboot by comparing the newest installed version against the booted OS version (IMAGE_VERSION=from os-release). When a component is selected via--component=, this ends up comparing the component's version against the unrelated host OS version; by design these live in separate version spaces, so the comparison is meaningless and reboot decisions become arbitrary: depending on the relative version strings, sysupdate either always reboots or never does.Example from the issue:
% /usr/lib/systemd/systemd-sysupdate --component=containerd reboot
Newest installed version '2.3.0' is older than booted version '20260527200656'.
This refuses the combination with a clear error instead of silently performing a bogus comparison:
verb_pending_or_reboot()rejects--component=for thepending/rebootverbs (mirroring the existing--root=/--image=rejection).verb_update_impl()rejects--rebootcombined with--component=, before any update work is done.Correctly tracking a per-component "booted" version (which could be "none", making a reboot always apply) is a larger feature and left for the future, as the reporter suggested.
The daemon (
sysupdated) andupdatectldon't perform this host-version comparison, so the change is confined to thesystemd-sysupdateCLI.Documentation and a negative regression test (TEST-72) are included.
AI-use Disclosure:
I took assistance from Claude Opus 4.8 to scope out the issue, help with writing proper comments/documentation, and help with writing the PR description.