sys-apps/systemd: sysupdate do not fail - #4118
Open
tormath1 wants to merge 4 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Gentoo user-patch for sys-apps/systemd to prevent systemd-sysupdate invocations (e.g., via timers/services) from exiting non-zero when no transfer definitions are present, addressing Flatcar issue #1979 where the resulting failed units are considered harmless noise.
Changes:
- Introduce a systemd patch intended to downgrade “no transfer definitions” from an error to a warning while returning success.
- Adjust
src/sysupdate/sysupdate.cbehavior (via patch) for the “no transfers defined” condition.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+4
| From f5f1ae2462f192825c5cfd050b33d093a68d8add Mon Sep 17 00:00:00 2001 | ||
| From: Mathieu Tortuyaux <mtortuyaux@microsoft.com> | ||
| Date: Tue, 23 Jun 2026 14:44:49 +0200 | ||
| Subject: [PATCH] sysupdate: do not fail when no transfers are defined |
Comment on lines
+9
to
+10
| We can still warn but this should not fail (i.e return an exit code | ||
| different from zero). |
Comment on lines
+4
to
+12
| Subject: [PATCH] sysupdate: do not fail when no transfers are defined | ||
|
|
||
| `sysupdate` operations like 'list', 'reboot', etc. should not fail when | ||
| no transfers are defined. | ||
|
|
||
| We can still warn but this should not fail (i.e return an exit code | ||
| different from zero). | ||
|
|
||
| Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com> |
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
June 23, 2026 16:29
fab43c4 to
142ed2c
Compare
Comment on lines
+46
to
+49
| + if (!context->newest_installed || context->n_transfers) { | ||
| + log_warning("Couldn't find any suitable installed versions."); | ||
| + return 0; | ||
| + } |
Contributor
There was a problem hiding this comment.
I'm not sufficiently familiar with this to say for sure whether Copilot is right here, but I think it might be, even though its other points weren't that helpful.
Contributor
Author
There was a problem hiding this comment.
Same here - what I like with the copilot approach is that it really preserves the initial behavior so we should not introduce side effects.
EDIT: Applied the proposal + kicked-off a CI
Contributor
Author
There was a problem hiding this comment.
@chewi that seems to do the trick. It sill works as expected and I think it might get higher chance to get approved upstream. I'll now proceed to upstream PR if it works for you.
Comment on lines
+29
to
+32
| + if (arg_component) { | ||
| + log_warning("No transfer definitions for component '%s' found.", arg_component); | ||
| + return 0; | ||
| + } |
Comment on lines
+9
to
+10
| We can still warn but this should not fail (i.e return an exit code | ||
| different from zero). |
tormath1
marked this pull request as ready for review
June 25, 2026 07:30
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
June 25, 2026 07:31
ac11b05 to
85c642d
Compare
Comment on lines
+1
to
+4
| From 948a3c664050d9f74b98e2b016cb810aaf4e9651 Mon Sep 17 00:00:00 2001 | ||
| From: Mathieu Tortuyaux <mtortuyaux@microsoft.com> | ||
| Date: Tue, 23 Jun 2026 14:44:49 +0200 | ||
| Subject: [PATCH] sysupdate: do not fail when no transfers are defined |
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
June 25, 2026 11:57
3395e94 to
cdcb555
Compare
chewi
approved these changes
Jun 25, 2026
Comment on lines
+1
to
+4
| From 45769b525e8350cda246427987c6349dfabfef1c Mon Sep 17 00:00:00 2001 | ||
| From: Philip Withnall <pwithnall@gnome.org> | ||
| Date: Thu, 21 May 2026 11:14:01 +0100 | ||
| Subject: [PATCH 1/4] sysupdate: Convert an internal bool argument to flags |
Comment on lines
+218
to
+221
| If `sysupdate` is called with various command line arguments to affect | ||
| how its configuration is loaded, do *not* say that a default component | ||
| exists, as these arguments essentially anull the possibility of a | ||
| default being used in that process. |
Comment on lines
+223
to
+227
| Add an integration test based on the reproducer provided by the issue | ||
| reporter. This test has been tested to fail if the changes to | ||
| `sysupdate.c` aren’t applied — if so, the second call to `sysupdate | ||
| components` would return | ||
| `{"default":true,"components":["some-component"]}`. |
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
June 26, 2026 13:31
b1452fc to
7b2e20c
Compare
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
July 24, 2026 08:02
7b2e20c to
a229ce2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/sysupdate.patch:221
- Typo in the commit message: “anull” isn’t a word; it reads like “annul” (or possibly “nullify”).
If `sysupdate` is called with various command line arguments to affect
how its configuration is loaded, do *not* say that a default component
exists, as these arguments essentially anull the possibility of a
default being used in that process.
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/sysupdate.patch:227
- This commit message says it “Add[s] an integration test”, but this patch only changes
src/sysupdate/sysupdate.c(no test file changes are included). That’s misleading for downstream tracking and for an eventual upstream submission.
Add an integration test based on the reproducer provided by the issue
reporter. This test has been tested to fail if the changes to
`sysupdate.c` aren’t applied — if so, the second call to `sysupdate
components` would return
`{"default":true,"components":["some-component"]}`.
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/sysupdate.patch:4
- This directory’s patches appear to follow a numbered
000N-*.patchnaming scheme (and are listed inREADME.md), but this new file is namedsysupdate.patchand isn’t referenced in the README. If patch ordering or documentation relies on that convention, consider renaming it to the next000N-...name and adding it to the README list.
From 45769b525e8350cda246427987c6349dfabfef1c Mon Sep 17 00:00:00 2001
From: Philip Withnall <pwithnall@gnome.org>
Date: Thu, 21 May 2026 11:14:01 +0100
Subject: [PATCH 1/4] sysupdate: Convert an internal bool argument to flags
Comment on lines
+270
to
+272
| + /* Does the system have at least one transfer file in /etc/sysupdate.d, which can be considered a | ||
| + * TARGET_HOST? See target_get_argument() in sysupdated.c */ | ||
| + has_default_component = (!arg_definitions && |
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
August 25, 2026 13:08
a229ce2 to
db15972
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/0001-sysupdate-do-not-fail-when-no-transfers-are-defined.patch:52
- The patch adds
context.n_transfers, butcontextis dereferenced as a pointer elsewhere in the same hunk (context->newest_installed). As written, this will fail to compile/apply cleanly because.is invalid on a pointer type.
- if (!context->newest_installed)
+ if (!context->newest_installed) {
+ if (context.n_transfers == 0) {
+ log_warning("Couldn't find any suitable installed versions.");
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/0001-sysupdate-do-not-fail-when-no-transfers-are-defined.patch:24
- The PR description says this should follow the upstream approach (systemd/systemd#42750), but that PR changes
context_read_definitions()to downgrade the “no transfer definitions” error to a warning (affecting all verbs) and only adds a small conditional inverb_pending_or_reboot(). This patch instead changes a fewcontext_make_offline()call sites to pass0flags, which is a different behavior and is likely incomplete (other verbs that still passREAD_DEFINITIONS_REQUIRES_ANY_TRANSFERSwould continue to fail).
r = context_make_offline(&context, loop_device ? loop_device->node : NULL,
- READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS);
+ /* read_definition_flags= */ 0);
if (r < 0)
Comment on lines
+1
to
+5
| From c57f7efee706ce3bd0f12e87036c2edbde442403 Mon Sep 17 00:00:00 2001 | ||
| From: Mathieu Tortuyaux <mtortuyaux@microsoft.com> | ||
| Date: Tue, 25 Aug 2026 15:05:52 +0200 | ||
| Subject: [PATCH] sysupdate: do not fail when no transfers are defined | ||
|
|
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
August 25, 2026 14:24
db15972 to
0e3bd59
Compare
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
August 25, 2026 14:25
0e3bd59 to
254512d
Compare
Comment on lines
+21
to
+33
| r = context_make_offline(&context, loop_device ? loop_device->node : NULL, | ||
| - READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS); | ||
| + /* read_definition_flags= */ 0); | ||
| if (r < 0) | ||
| return r; | ||
|
|
||
| @@ -1642,7 +1642,7 @@ static int verb_vacuum(int argc, char *argv[], uintptr_t _data, void *userdata) | ||
| return r; | ||
|
|
||
| r = context_make_offline(&context, loop_device ? loop_device->node : NULL, | ||
| - READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS); | ||
| + /* read_definitions_flags= */ 0); | ||
| if (r < 0) |
Comment on lines
+18
to
+43
| @@ -1373,7 +1373,7 @@ static int verb_features(int argc, char *argv[], uintptr_t _data, void *userdata | ||
| return r; | ||
|
|
||
| r = context_make_offline(&context, loop_device ? loop_device->node : NULL, | ||
| - READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS); | ||
| + /* read_definition_flags= */ 0); | ||
| if (r < 0) | ||
| return r; | ||
|
|
||
| @@ -1642,7 +1642,7 @@ static int verb_vacuum(int argc, char *argv[], uintptr_t _data, void *userdata) | ||
| return r; | ||
|
|
||
| r = context_make_offline(&context, loop_device ? loop_device->node : NULL, | ||
| - READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS); | ||
| + /* read_definitions_flags= */ 0); | ||
| if (r < 0) | ||
| return r; | ||
|
|
||
| @@ -1665,7 +1665,7 @@ static int verb_pending_or_reboot(int argc, char *argv[], uintptr_t _data, void | ||
| "The --root=/--image= switches may not be combined with the '%s' operation.", argv[0]); | ||
|
|
||
| r = context_make_offline(&context, /* node= */ NULL, | ||
| - READ_DEFINITIONS_REQUIRES_ENABLED_TRANSFERS | READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS); | ||
| + READ_DEFINITIONS_REQUIRES_ENABLED_TRANSFERS); | ||
| if (r < 0) | ||
| return r; |
Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
tormath1
force-pushed
the
tormath1/sysupdate
branch
from
August 27, 2026 07:44
254512d to
9cb2a2d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/0001-sysupdate-do-not-fail-when-no-transfers-are-defined.patch:4
- This patch is named
0001-…, but the same directory already contains0001-wait-online-set-any-by-default.patchand then0002…0008. Having two0001patches makes the series ordering non-obvious and can change the actual application order (lexicographic) vs the intended sequence.
From c57f7efee706ce3bd0f12e87036c2edbde442403 Mon Sep 17 00:00:00 2001
From: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
Date: Tue, 25 Aug 2026 15:05:52 +0200
Subject: [PATCH] sysupdate: do not fail when no transfers are defined
sdk_container/src/third_party/coreos-overlay/coreos/user-patches/sys-apps/systemd/0001-sysupdate-do-not-fail-when-no-transfers-are-defined.patch:33
- The added continuation line has inconsistent indentation and a slightly different parameter name in the comment (
read_definitions_flagsvsread_definition_flags). Since this line ends up in the patched C source, keep it consistent (or drop the comment entirely).
r = context_make_offline(&context, loop_device ? loop_device->node : NULL,
- READ_DEFINITIONS_REQUIRES_ANY_TRANSFERS);
+ /* read_definitions_flags= */ 0);
if (r < 0)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attempt to solve: flatcar/Flatcar#1979 with an upstream approach.
TODO:
CI ✔️ : https://jenkins.flatcar.org/job/container/job/packages_all_arches/589/cldsv/