From 79348036342e80eefd59e9828c664b95c19319bc Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 21 Jun 2024 20:03:12 -0400 Subject: [PATCH 1/5] sysupdate: Fix resource_find_instance The current implementation will never find a match, because in the event of a match instance_cmp falls through to comparing paths and the key we're matching against will always have a path of NULL. So let's just use a separate compare function, just to make sure future updates to instance_cmp don't break resource_find_instance again. --- src/sysupdate/sysupdate-resource.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/sysupdate/sysupdate-resource.c b/src/sysupdate/sysupdate-resource.c index 5b7aee2b54..9430899897 100644 --- a/src/sysupdate/sysupdate-resource.c +++ b/src/sysupdate/sysupdate-resource.c @@ -527,13 +527,25 @@ int resource_load_instances(Resource *rr, bool verify, Hashmap **web_cache) { return 0; } +static int instance_version_match(Instance *const*a, Instance *const*b) { + assert(a); + assert(b); + assert(*a); + assert(*b); + assert((*a)->metadata.version); + assert((*b)->metadata.version); + + /* List is sorted newest-to-oldest */ + return -strverscmp_improved((*a)->metadata.version, (*b)->metadata.version); +} + Instance* resource_find_instance(Resource *rr, const char *version) { Instance key = { .metadata.version = (char*) version, }, *k = &key; Instance **found; - found = typesafe_bsearch(&k, rr->instances, rr->n_instances, instance_cmp); + found = typesafe_bsearch(&k, rr->instances, rr->n_instances, instance_version_match); if (!found) return NULL; From 0402bf682fb3b11bd4fd36969abf5426f24c3dde Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Fri, 21 Jun 2024 20:49:48 -0400 Subject: [PATCH 2/5] sysupdate: Check that --instances-max is in bounds Otherwise user can pass in --instances-max=0 and crash sysupdate with an assertion failure. --- man/systemd-sysupdate.xml | 6 +++--- src/sysupdate/sysupdate.c | 8 ++++++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/man/systemd-sysupdate.xml b/man/systemd-sysupdate.xml index dffe835c04..f57a17b79a 100644 --- a/man/systemd-sysupdate.xml +++ b/man/systemd-sysupdate.xml @@ -257,9 +257,9 @@ - Takes a decimal integer greater than or equal to 2. Controls how many versions to - keep at any time. This option may also be configured inside the transfer files, via the - InstancesMax= setting, see + Takes a decimal integer greater than or equal to 2 while updating or 1 while vacuuming. + Controls how many versions to keep at any time. This option may also be configured inside the transfer + files, via the InstancesMax= setting, see sysupdate.d5 for details. diff --git a/src/sysupdate/sysupdate.c b/src/sysupdate/sysupdate.c index dee8348bdb..5854535427 100644 --- a/src/sysupdate/sysupdate.c +++ b/src/sysupdate/sysupdate.c @@ -1067,6 +1067,10 @@ static int verb_vacuum(int argc, char **argv, void *userdata) { assert(argc <= 1); + if (arg_instances_max < 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "The --instances-max argument must be >= 1 while vacuuming"); + r = process_image(/* ro= */ false, &mounted_dir, &loop_device); if (r < 0) return r; @@ -1090,6 +1094,10 @@ static int verb_update(int argc, char **argv, void *userdata) { assert(argc <= 2); version = argc >= 2 ? argv[1] : NULL; + if (arg_instances_max < 2) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "The --instances-max argument must be >= 2 while updating"); + if (arg_reboot) { /* If automatic reboot on completion is requested, let's first determine the currently booted image */ From 57ada07e7aa2232eeedd75367ac55ef5bcf65f75 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Tue, 2 Jul 2024 12:13:45 -0400 Subject: [PATCH 3/5] sysupdate: Track incompletely-installed versions When enumerating what versions exist for a given target, sysupdate would completely throw out any version that's incomplete (where some of the transfers in the target have that version installed or available, and other transfers do not). If we're trying to find what versions we can offer for download, this is great behavior. If the server side is advertising a partial update to download, we shouldn't present it to the user. On the other hand, if we're enumerating what versions we have currently installed, this is a bad behavior. It makes sysupdate fragile. For example, if a sysext introduces a new .conf file into /usr/lib/sysupdate.d, suddenly the currently-installed OS stops being a version that we've enumerated. Since it's not enumerated, it's not protected, and so sysupdate will wipe the booted OS. So if we're looking for installed versions, we now loosen the restrictions and enumerate incomplete installations. Partial fix for https://github.com/systemd/systemd/issues/33339 --- man/org.freedesktop.sysupdate1.xml | 7 ++ src/sysupdate/sysupdate-update-set-flags.c | 44 ++++++++ src/sysupdate/sysupdate-update-set-flags.h | 11 +- src/sysupdate/sysupdate.c | 115 +++++++++++++-------- src/sysupdate/updatectl.c | 5 +- 5 files changed, 134 insertions(+), 48 deletions(-) diff --git a/man/org.freedesktop.sysupdate1.xml b/man/org.freedesktop.sysupdate1.xml index 66c21fab3d..3acae3d701 100644 --- a/man/org.freedesktop.sysupdate1.xml +++ b/man/org.freedesktop.sysupdate1.xml @@ -222,6 +222,13 @@ node /org/freedesktop/sysupdate1/target/host { Vacuum() operation. + + incomplete + A boolean indicating whether this version is incomplete, which means that it is + missing some file. Note that only installed incomplete versions will be offered by the service; + versions that are incomplete on the server-side are completely ignored. + + changelog_urls A list of strings that contain user-presentable URLs to ChangeLogs associated with diff --git a/src/sysupdate/sysupdate-update-set-flags.c b/src/sysupdate/sysupdate-update-set-flags.c index d9232e4a17..d37363e073 100644 --- a/src/sysupdate/sysupdate-update-set-flags.c +++ b/src/sysupdate/sysupdate-update-set-flags.c @@ -10,6 +10,9 @@ const char* update_set_flags_to_color(UpdateSetFlags flags) { if (flags == 0 || (flags & UPDATE_OBSOLETE)) return (flags & UPDATE_NEWEST) ? ansi_highlight_grey() : ansi_grey(); + if (FLAGS_SET(flags, UPDATE_INSTALLED|UPDATE_INCOMPLETE)) + return ansi_highlight_yellow(); + if (FLAGS_SET(flags, UPDATE_INSTALLED|UPDATE_NEWEST)) return ansi_highlight(); @@ -68,6 +71,27 @@ const char* update_set_flags_to_string(UpdateSetFlags flags) { case UPDATE_AVAILABLE|UPDATE_PROTECTED: return "available"; + case UPDATE_INSTALLED|UPDATE_INCOMPLETE|UPDATE_NEWEST: + case UPDATE_INSTALLED|UPDATE_INCOMPLETE|UPDATE_NEWEST|UPDATE_PROTECTED: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_INCOMPLETE|UPDATE_NEWEST: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_INCOMPLETE|UPDATE_NEWEST|UPDATE_PROTECTED: + return "current+incomplete"; + + case UPDATE_INSTALLED|UPDATE_INCOMPLETE: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_INCOMPLETE: + return "installed+incomplete"; + + case UPDATE_INSTALLED|UPDATE_INCOMPLETE|UPDATE_PROTECTED: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_INCOMPLETE|UPDATE_PROTECTED: + return "protected+incomplete"; + + case UPDATE_AVAILABLE|UPDATE_INCOMPLETE: + case UPDATE_AVAILABLE|UPDATE_INCOMPLETE|UPDATE_PROTECTED: + case UPDATE_AVAILABLE|UPDATE_INCOMPLETE|UPDATE_NEWEST: + case UPDATE_AVAILABLE|UPDATE_INCOMPLETE|UPDATE_NEWEST|UPDATE_PROTECTED: + /* We must never offer an update as available for download if it's incomplete */ + assert_not_reached(); + case UPDATE_INSTALLED|UPDATE_OBSOLETE|UPDATE_NEWEST: case UPDATE_INSTALLED|UPDATE_OBSOLETE|UPDATE_NEWEST|UPDATE_PROTECTED: case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_NEWEST: @@ -88,6 +112,26 @@ const char* update_set_flags_to_string(UpdateSetFlags flags) { case UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_NEWEST|UPDATE_PROTECTED: return "available+obsolete"; + case UPDATE_INSTALLED|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_NEWEST: + case UPDATE_INSTALLED|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_NEWEST|UPDATE_PROTECTED: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_NEWEST: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_NEWEST|UPDATE_PROTECTED: + return "current+obsolete+incomplete"; + + case UPDATE_INSTALLED|UPDATE_OBSOLETE|UPDATE_INCOMPLETE: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE: + return "installed+obsolete+incomplete"; + + case UPDATE_INSTALLED|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_PROTECTED: + case UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_PROTECTED: + return "protected+obsolete+incomplete"; + + case UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE: + case UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_PROTECTED: + case UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_NEWEST: + case UPDATE_AVAILABLE|UPDATE_OBSOLETE|UPDATE_INCOMPLETE|UPDATE_NEWEST|UPDATE_PROTECTED: + assert_not_reached(); + default: assert_not_reached(); } diff --git a/src/sysupdate/sysupdate-update-set-flags.h b/src/sysupdate/sysupdate-update-set-flags.h index 49ac5e1835..881dfdefff 100644 --- a/src/sysupdate/sysupdate-update-set-flags.h +++ b/src/sysupdate/sysupdate-update-set-flags.h @@ -2,11 +2,12 @@ #pragma once typedef enum UpdateSetFlags { - UPDATE_NEWEST = 1 << 0, - UPDATE_AVAILABLE = 1 << 1, - UPDATE_INSTALLED = 1 << 2, - UPDATE_OBSOLETE = 1 << 3, - UPDATE_PROTECTED = 1 << 4, + UPDATE_NEWEST = 1 << 0, + UPDATE_AVAILABLE = 1 << 1, + UPDATE_INSTALLED = 1 << 2, + UPDATE_OBSOLETE = 1 << 3, + UPDATE_PROTECTED = 1 << 4, + UPDATE_INCOMPLETE = 1 << 5, } UpdateSetFlags; const char* update_set_flags_to_color(UpdateSetFlags flags); diff --git a/src/sysupdate/sysupdate.c b/src/sysupdate/sysupdate.c index 5854535427..9b56bf1a8f 100644 --- a/src/sysupdate/sysupdate.c +++ b/src/sysupdate/sysupdate.c @@ -215,7 +215,6 @@ static int context_load_available_instances(Context *c) { } static int context_discover_update_sets_by_flag(Context *c, UpdateSetFlags flags) { - _cleanup_free_ Instance **cursor_instances = NULL; _cleanup_free_ char *boundary = NULL; bool newest_found = false; int r; @@ -224,64 +223,91 @@ static int context_discover_update_sets_by_flag(Context *c, UpdateSetFlags flags assert(IN_SET(flags, UPDATE_AVAILABLE, UPDATE_INSTALLED)); for (;;) { - bool incomplete = false, exists = false; + _cleanup_free_ Instance **cursor_instances = NULL; + bool skip = false; UpdateSetFlags extra_flags = 0; _cleanup_free_ char *cursor = NULL; UpdateSet *us = NULL; - for (size_t k = 0; k < c->n_transfers; k++) { - Transfer *t = c->transfers[k]; - bool cursor_found = false; + /* First, let's find the newest version that's older than the boundary. */ + FOREACH_ARRAY(tr, c->transfers, c->n_transfers) { Resource *rr; - assert(t); + assert(*tr); if (flags == UPDATE_AVAILABLE) - rr = &t->source; + rr = &(*tr)->source; else { assert(flags == UPDATE_INSTALLED); - rr = &t->target; + rr = &(*tr)->target; } FOREACH_ARRAY(inst, rr->instances, rr->n_instances) { - Instance *i = *inst; + Instance *i = *inst; /* Sorted newest-to-oldest */ + assert(i); - /* Is the instance we are looking at equal or newer than the boundary? If so, we - * already checked this version, and it wasn't complete, let's ignore it. */ if (boundary && strverscmp_improved(i->metadata.version, boundary) >= 0) - continue; + continue; /* Not older than the boundary */ - if (cursor) { - if (strverscmp_improved(i->metadata.version, cursor) != 0) - continue; - } else { - cursor = strdup(i->metadata.version); - if (!cursor) - return log_oom(); - } + if (cursor && strverscmp(i->metadata.version, cursor) <= 0) + break; /* Not newer than the cursor. The same will be true for all + * subsequent instances (due to sorting) so let's skip to the + * next transfer. */ - cursor_found = true; + if (free_and_strdup(&cursor, i->metadata.version) < 0) + return log_oom(); - if (!cursor_instances) { - cursor_instances = new(Instance*, c->n_transfers); - if (!cursor_instances) - return -ENOMEM; - } - cursor_instances[k] = i; - break; + break; /* All subsequent instances will be older than this one */ } - if (!cursor) /* No suitable instance beyond the boundary found? Then we are done! */ - break; + if (flags == UPDATE_AVAILABLE && !cursor) + break; /* This transfer didn't have a version older than the boundary, + * so any older-than-boundary version that might exist in a different + * transfer must always be incomplete. For reasons described below, + * we don't include incomplete versions for AVAILABLE updates. So we + * are completely done looking. */ + } - if (!cursor_found) { - /* Hmm, we didn't find the version indicated by 'cursor' among the instances - * of this transfer, let's skip it. */ - incomplete = true; - break; + if (!cursor) /* We didn't find anything older than the boundary, so we're done. */ + break; + + cursor_instances = new0(Instance*, c->n_transfers); + if (!cursor_instances) + return log_oom(); + + /* Now let's find all the instances that match the version of the cursor, if we have them */ + for (size_t k = 0; k < c->n_transfers; k++) { + Transfer *t = c->transfers[k]; + Instance *match = NULL; + + assert(t); + + if (flags == UPDATE_AVAILABLE) { + match = resource_find_instance(&t->source, cursor); + if (!match) { + /* When we're looking for updates to download, we don't offer + * incomplete versions at all. The server wants to send us an update + * with parts of the OS missing. For robustness sake, let's not do + * that. */ + skip = true; + break; + } + } else { + assert(flags == UPDATE_INSTALLED); + + match = resource_find_instance(&t->target, cursor); + if (!match) { + /* When we're looking for installed versions, let's be robust and treat + * an incomplete installation as an installation. Otherwise, there are + * situations that can lead to sysupdate wiping the currently booted OS. + * See https://github.com/systemd/systemd/issues/33339 */ + extra_flags |= UPDATE_INCOMPLETE; + } } + cursor_instances[k] = match; + if (t->min_version && strverscmp_improved(t->min_version, cursor) > 0) extra_flags |= UPDATE_OBSOLETE; @@ -289,14 +315,11 @@ static int context_discover_update_sets_by_flag(Context *c, UpdateSetFlags flags extra_flags |= UPDATE_PROTECTED; } - if (!cursor) /* EOL */ - break; - r = free_and_strdup_warn(&boundary, cursor); if (r < 0) return r; - if (incomplete) /* One transfer was missing this version, ignore the whole thing */ + if (skip) continue; /* See if we already have this update set in our table */ @@ -307,12 +330,12 @@ static int context_discover_update_sets_by_flag(Context *c, UpdateSetFlags flags /* We only store the instances we found first, but we remember we also found it again */ u->flags |= flags | extra_flags; - exists = true; + skip = true; newest_found = true; break; } - if (exists) + if (skip) continue; /* Doesn't exist yet, let's add it */ @@ -504,6 +527,12 @@ static int context_show_version(Context *c, const char *version) { FOREACH_ARRAY(inst, us->instances, us->n_instances) { Instance *i = *inst; + + if (!i) { + assert(FLAGS_SET(us->flags, UPDATE_INCOMPLETE)); + continue; + } + r = table_add_many(t, TABLE_STRING, resource_type_to_string(i->resource->type), TABLE_PATH, i->path); @@ -641,13 +670,14 @@ static int context_show_version(Context *c, const char *version) { if (FLAGS_SET(arg_json_format_flags, SD_JSON_FORMAT_OFF)) { printf("%s%s%s Version: %s\n" " State: %s%s%s\n" - "Installed: %s%s\n" + "Installed: %s%s%s%s%s\n" "Available: %s%s\n" "Protected: %s%s%s\n" " Obsolete: %s%s%s\n", strempty(update_set_flags_to_color(us->flags)), update_set_flags_to_glyph(us->flags), ansi_normal(), us->version, strempty(update_set_flags_to_color(us->flags)), update_set_flags_to_string(us->flags), ansi_normal(), yes_no(us->flags & UPDATE_INSTALLED), FLAGS_SET(us->flags, UPDATE_INSTALLED|UPDATE_NEWEST) ? " (newest)" : "", + FLAGS_SET(us->flags, UPDATE_INCOMPLETE) ? ansi_highlight_yellow() : "", FLAGS_SET(us->flags, UPDATE_INCOMPLETE) ? " (incomplete)" : "", ansi_normal(), yes_no(us->flags & UPDATE_AVAILABLE), (us->flags & (UPDATE_INSTALLED|UPDATE_AVAILABLE|UPDATE_NEWEST)) == (UPDATE_AVAILABLE|UPDATE_NEWEST) ? " (newest)" : "", FLAGS_SET(us->flags, UPDATE_INSTALLED|UPDATE_PROTECTED) ? ansi_highlight() : "", yes_no(FLAGS_SET(us->flags, UPDATE_INSTALLED|UPDATE_PROTECTED)), ansi_normal(), us->flags & UPDATE_OBSOLETE ? ansi_highlight_red() : "", yes_no(us->flags & UPDATE_OBSOLETE), ansi_normal()); @@ -675,6 +705,7 @@ static int context_show_version(Context *c, const char *version) { SD_JSON_BUILD_PAIR_BOOLEAN("installed", FLAGS_SET(us->flags, UPDATE_INSTALLED)), SD_JSON_BUILD_PAIR_BOOLEAN("obsolete", FLAGS_SET(us->flags, UPDATE_OBSOLETE)), SD_JSON_BUILD_PAIR_BOOLEAN("protected", FLAGS_SET(us->flags, UPDATE_PROTECTED)), + SD_JSON_BUILD_PAIR_BOOLEAN("incomplete", FLAGS_SET(us->flags, UPDATE_INCOMPLETE)), SD_JSON_BUILD_PAIR_STRV("changelog_urls", changelog_urls), SD_JSON_BUILD_PAIR_VARIANT("contents", t_json)); if (r < 0) diff --git a/src/sysupdate/updatectl.c b/src/sysupdate/updatectl.c index 151fa9e1d7..d8db49e57c 100644 --- a/src/sysupdate/updatectl.c +++ b/src/sysupdate/updatectl.c @@ -324,7 +324,8 @@ static int parse_describe(sd_bus_message *reply, Version *ret) { Version v = {}; char *version_json = NULL; _cleanup_(sd_json_variant_unrefp) sd_json_variant *json = NULL, *contents_json = NULL; - bool newest = false, available = false, installed = false, obsolete = false, protected = false; + bool newest = false, available = false, installed = false, obsolete = false, protected = false, + incomplete = false; int r; assert(reply); @@ -348,6 +349,7 @@ static int parse_describe(sd_bus_message *reply, Version *ret) { { "installed", SD_JSON_VARIANT_BOOLEAN, sd_json_dispatch_stdbool, PTR_TO_SIZE(&installed), 0 }, { "obsolete", SD_JSON_VARIANT_BOOLEAN, sd_json_dispatch_stdbool, PTR_TO_SIZE(&obsolete), 0 }, { "protected", SD_JSON_VARIANT_BOOLEAN, sd_json_dispatch_stdbool, PTR_TO_SIZE(&protected), 0 }, + { "incomplete", SD_JSON_VARIANT_BOOLEAN, sd_json_dispatch_stdbool, PTR_TO_SIZE(&incomplete), 0 }, { "changelog_urls", SD_JSON_VARIANT_ARRAY, sd_json_dispatch_strv, PTR_TO_SIZE(&v.changelog), 0 }, { "contents", SD_JSON_VARIANT_ARRAY, sd_json_dispatch_variant, PTR_TO_SIZE(&contents_json), 0 }, {}, @@ -362,6 +364,7 @@ static int parse_describe(sd_bus_message *reply, Version *ret) { SET_FLAG(v.flags, UPDATE_INSTALLED, installed); SET_FLAG(v.flags, UPDATE_OBSOLETE, obsolete); SET_FLAG(v.flags, UPDATE_PROTECTED, protected); + SET_FLAG(v.flags, UPDATE_INCOMPLETE, incomplete); r = sd_json_variant_format(contents_json, 0, &v.contents_json); if (r < 0) From 631803cccd8112b420ad74195d78b0f1ae029ac7 Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Wed, 19 Jun 2024 21:05:04 -0400 Subject: [PATCH 4/5] sysupdate: Repair incomplete versions in-place A previous commit made sysupdate recognize installed versions where some transfers are missing. This commit teaches sysupdate how to correctly repair these incomplete versions. Previously, if you had a incomplete installation of the OS booted, and ran sysupdate in an attempt to repair it, sysupdate would make things worse by creating copies of the currently-booted partitions in the inactive slots. Then at boot you have two identical partitions, with identical labels an UUIDs, and end up with a mess. With this commit, sysupdate is able to recognize situations where it can simply download the missing transfers and leave the rest of the system undistrubed. Partial fix for https://github.com/systemd/systemd/issues/33339 --- man/org.freedesktop.sysupdate1.xml | 3 +- src/sysupdate/sysupdate-transfer.c | 3 +- src/sysupdate/sysupdate.c | 68 ++++++++++++++++++++++++++---- 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/man/org.freedesktop.sysupdate1.xml b/man/org.freedesktop.sysupdate1.xml index 3acae3d701..79f718f93c 100644 --- a/man/org.freedesktop.sysupdate1.xml +++ b/man/org.freedesktop.sysupdate1.xml @@ -226,7 +226,8 @@ node /org/freedesktop/sysupdate1/target/host { incomplete A boolean indicating whether this version is incomplete, which means that it is missing some file. Note that only installed incomplete versions will be offered by the service; - versions that are incomplete on the server-side are completely ignored. + versions that are incomplete on the server-side are completely ignored. Incomplete versions can + be repaired in-place by calling Update() on that version. diff --git a/src/sysupdate/sysupdate-transfer.c b/src/sysupdate/sysupdate-transfer.c index 69bba00cab..f7d9a043fc 100644 --- a/src/sysupdate/sysupdate-transfer.c +++ b/src/sysupdate/sysupdate-transfer.c @@ -1075,8 +1075,7 @@ int transfer_acquire_instance(Transfer *t, Instance *i, TransferProgress cb, voi assert(t); assert(i); - assert(i->resource); - assert(t == container_of(i->resource, Transfer, source)); + assert(i->resource == &t->source); assert(cb); /* Does this instance already exist in the target? Then we don't need to acquire anything */ diff --git a/src/sysupdate/sysupdate.c b/src/sysupdate/sysupdate.c index 9b56bf1a8f..04fd003299 100644 --- a/src/sysupdate/sysupdate.c +++ b/src/sysupdate/sysupdate.c @@ -325,11 +325,36 @@ static int context_discover_update_sets_by_flag(Context *c, UpdateSetFlags flags /* See if we already have this update set in our table */ FOREACH_ARRAY(update_set, c->update_sets, c->n_update_sets) { UpdateSet *u = *update_set; + if (strverscmp_improved(u->version, cursor) != 0) continue; - /* We only store the instances we found first, but we remember we also found it again */ + /* Merge in what we've learned and continue onto the next version */ + + if (FLAGS_SET(u->flags, UPDATE_INCOMPLETE)) { + assert(u->n_instances == c->n_transfers); + + /* Incomplete updates will have picked NULL instances for the transfers that + * are missing. Now we have more information, so let's try to fill them in. */ + + for (size_t j = 0; j < u->n_instances; j++) { + if (!u->instances[j]) + u->instances[j] = cursor_instances[j]; + + /* Make sure that the list is full if the update is AVAILABLE */ + assert(flags != UPDATE_AVAILABLE || u->instances[j]); + } + } + u->flags |= flags | extra_flags; + + /* If this is the newest installed version, that is incomplete and just became marked + * as available, and if there is no other candidate available, we promote this to be + * the candidate. */ + if (FLAGS_SET(u->flags, UPDATE_NEWEST|UPDATE_INSTALLED|UPDATE_INCOMPLETE|UPDATE_AVAILABLE) && + !c->candidate && !FLAGS_SET(u->flags, UPDATE_OBSOLETE)) + c->candidate = u; + skip = true; newest_found = true; break; @@ -367,7 +392,8 @@ static int context_discover_update_sets_by_flag(Context *c, UpdateSetFlags flags } /* Newest installed is newer than or equal to candidate? Then suppress the candidate */ - if (c->newest_installed && c->candidate && strverscmp_improved(c->newest_installed->version, c->candidate->version) >= 0) + if (c->newest_installed && !FLAGS_SET(c->newest_installed->flags, UPDATE_INCOMPLETE) && + c->candidate && strverscmp_improved(c->newest_installed->version, c->candidate->version) >= 0) c->candidate = NULL; return 0; @@ -736,6 +762,10 @@ static int context_vacuum( FOREACH_ARRAY(tr, c->transfers, c->n_transfers) { Transfer *t = *tr; + /* Don't bother clearing out space if we're not going to be downloading anything */ + if (extra_protected_version && resource_find_instance(&t->target, extra_protected_version)) + continue; + r = transfer_vacuum(t, space, extra_protected_version); if (r < 0) return r; @@ -866,7 +896,9 @@ static int context_apply( us = c->candidate; } - if (FLAGS_SET(us->flags, UPDATE_INSTALLED)) { + if (FLAGS_SET(us->flags, UPDATE_INCOMPLETE)) + log_info("Selected update '%s' is already installed, but incomplete. Repairing.", us->version); + else if (FLAGS_SET(us->flags, UPDATE_INSTALLED)) { log_info("Selected update '%s' is already installed. Skipping update.", us->version); if (ret_applied) @@ -874,13 +906,12 @@ static int context_apply( return 0; } + if (!FLAGS_SET(us->flags, UPDATE_AVAILABLE)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Selected update '%s' is not available, refusing.", us->version); if (FLAGS_SET(us->flags, UPDATE_OBSOLETE)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Selected update '%s' is obsolete, refusing.", us->version); - assert((us->flags & (UPDATE_AVAILABLE|UPDATE_INSTALLED|UPDATE_OBSOLETE)) == UPDATE_AVAILABLE); - if (!FLAGS_SET(us->flags, UPDATE_NEWEST)) log_notice("Selected update '%s' is not the newest, proceeding anyway.", us->version); if (c->newest_installed && strverscmp_improved(c->newest_installed->version, us->version) > 0) @@ -913,8 +944,17 @@ static int context_apply( assert(us->n_instances == c->n_transfers); for (size_t i = 0; i < c->n_transfers; i++) { - r = transfer_acquire_instance(c->transfers[i], us->instances[i], - context_on_acquire_progress, c); + Instance *inst = us->instances[i]; + Transfer *t = c->transfers[i]; + + assert(inst); /* ditto */ + + if (inst->resource == &t->target) { /* a present transfer in an incomplete installation */ + assert(FLAGS_SET(us->flags, UPDATE_INCOMPLETE)); + continue; + } + + r = transfer_acquire_instance(t, inst, context_on_acquire_progress, c); if (r < 0) return r; } @@ -926,7 +966,13 @@ static int context_apply( "STATUS=Installing '%s'.", us->version); for (size_t i = 0; i < c->n_transfers; i++) { - r = transfer_install_instance(c->transfers[i], us->instances[i], arg_root); + Instance *inst = us->instances[i]; + Transfer *t = c->transfers[i]; + + if (inst->resource == &t->target) + continue; + + r = transfer_install_instance(t, inst, arg_root); if (r < 0) return r; } @@ -1160,6 +1206,12 @@ static int verb_update(int argc, char **argv, void *userdata) { return reboot_now(); } + if (strverscmp_improved(applied->version, booted_version) == 0 && + FLAGS_SET(applied->flags, UPDATE_INCOMPLETE)) { + log_notice("Currently booted version was incomplete and has been repaired, rebooting."); + return reboot_now(); + } + log_info("Booted version is newer or identical to newly installed version, not rebooting."); } From e7416c9d4281af779bd09c8b600e7e9bf50ae10d Mon Sep 17 00:00:00 2001 From: Adrian Vovk Date: Tue, 2 Jul 2024 12:36:37 -0400 Subject: [PATCH 5/5] sysupdate: Add tests for incomplete versions To make sure we don't regress on #33339 --- test/units/TEST-72-SYSUPDATE.sh | 64 ++++++++++++++++++++++++++------- 1 file changed, 51 insertions(+), 13 deletions(-) diff --git a/test/units/TEST-72-SYSUPDATE.sh b/test/units/TEST-72-SYSUPDATE.sh index 3f4aa5ce8a..4dcdf5dced 100755 --- a/test/units/TEST-72-SYSUPDATE.sh +++ b/test/units/TEST-72-SYSUPDATE.sh @@ -53,6 +53,10 @@ at_exit() { trap at_exit EXIT +update_checksums() { + (cd "$WORKDIR/source" && sha256sum uki* part* dir-*.tar.gz >SHA256SUMS) +} + new_version() { local sector_size="${1:?}" local version="${2:?}" @@ -74,7 +78,7 @@ new_version() { echo $RANDOM >"$WORKDIR/source/dir-$version/bar.txt" tar --numeric-owner -C "$WORKDIR/source/dir-$version/" -czf "$WORKDIR/source/dir-$version.tar.gz" . - (cd "$WORKDIR/source" && sha256sum uki* part* dir-*.tar.gz >SHA256SUMS) + update_checksums } update_now() { @@ -91,9 +95,9 @@ verify_version() { local sector_size="${2:?}" local version="${3:?}" local part1_number="${4:?}" - local part2_number="${5:?}" - local gpt_reserved_sectors part1_offset part2_offset + local gpt_reserved_sectors part2_number part1_offset part2_offset + part2_number=$(( part1_number + 2 )) gpt_reserved_sectors=$((1024 * 1024 / sector_size)) part1_offset=$(((part1_number - 1) * 2048 + gpt_reserved_sectors)) part2_offset=$(((part2_number - 1) * 2048 + gpt_reserved_sectors)) @@ -108,6 +112,12 @@ verify_version() { # Check the extra efi cmp "$WORKDIR/source/uki-extra-$version.efi" "$WORKDIR/xbootldr/EFI/Linux/uki_$version.efi.extra.d/extra.addon.efi" +} + +verify_version_current() { + local version="${3:?}" + + verify_version "$@" # Check the directories cmp "$WORKDIR/source/dir-$version/foo.txt" "$WORKDIR/dirs/current/foo.txt" @@ -225,24 +235,50 @@ EOF # Install initial version and verify new_version "$sector_size" v1 update_now - verify_version "$blockdev" "$sector_size" v1 1 3 + verify_version_current "$blockdev" "$sector_size" v1 1 # Create second version, update and verify that it is added new_version "$sector_size" v2 update_now - verify_version "$blockdev" "$sector_size" v2 2 4 + verify_version "$blockdev" "$sector_size" v1 1 + verify_version_current "$blockdev" "$sector_size" v2 2 # Create third version, update and verify it replaced the first version new_version "$sector_size" v3 update_now - verify_version "$blockdev" "$sector_size" v3 1 3 + verify_version_current "$blockdev" "$sector_size" v3 1 + verify_version "$blockdev" "$sector_size" v2 2 test ! -f "$WORKDIR/xbootldr/EFI/Linux/uki_v1+3-0.efi" test ! -f "$WORKDIR/xbootldr/EFI/Linux/uki_v1.efi.extra.d/extra.addon.efi" test ! -d "$WORKDIR/xbootldr/EFI/Linux/uki_v1.efi.extra.d" - # Create fourth version, update using updatectl and verify it replaced the - # second version + # Create fourth version, but make it be incomplete (i.e. missing some files) + # on the server-side. Verify that it's not offered as an update. new_version "$sector_size" v4 + rm "$WORKDIR/source/uki-extra-v4.efi" + update_checksums + (! "$SYSUPDATE" --verify=no check-new) + + # Create a fifth version, that's complete on the server side. We should + # completely skip the incomplete v4 and install v5 instead. + new_version "$sector_size" v5 + update_now + verify_version "$blockdev" "$sector_size" v3 1 + verify_version_current "$blockdev" "$sector_size" v5 2 + + # Make the local installation of v5 incomplete by deleting a file, then make + # sure that sysupdate still recognizes the installation and can complete it + # in place + rm -r "$WORKDIR/xbootldr/EFI/Linux/uki_v5.efi.extra.d" + "$SYSUPDATE" --offline list v5 | grep -q "incomplete" + update_now + "$SYSUPDATE" --offline list v5 | grep -qv "incomplete" + verify_version "$blockdev" "$sector_size" v3 1 + verify_version_current "$blockdev" "$sector_size" v5 2 + + # Create sixth version, update using updatectl and verify it replaced the + # correct version + new_version "$sector_size" v6 if [[ -x "$SYSUPDATED" ]] && command -v updatectl; then systemctl start systemd-sysupdated "$SYSUPDATE" --verify=no check-new @@ -254,7 +290,8 @@ EOF # User-facing updatectl returns 0 if there's no updates, so use the low-level # utility to make sure we did upgrade (! "$SYSUPDATE" --verify=no check-new ) - verify_version "$blockdev" "$sector_size" v4 2 4 + verify_version_current "$blockdev" "$sector_size" v6 1 + verify_version "$blockdev" "$sector_size" v5 2 # Next, let's run updatectl's various inspection commands. We're not # testing for specific output, but this will at least catch obvious crashes @@ -265,18 +302,18 @@ EOF cp "$CONFIGDIR/01-first.conf" /run/sysupdate.test.d/01-first.conf updatectl list updatectl list host - updatectl list host@v4 + updatectl list host@v6 updatectl check rm -r /run/sysupdate.test.d fi - # Create fifth version, and update through a file:// URL. This should be + # Create seventh version, and update through a file:// URL. This should be # almost as good as testing HTTP, but is simpler for us to set up. file:// is # abstracted in curl for us, and since our main goal is to test our own code # (and not curl) this test should be quite good even if not comprehensive. This # will test the SHA256SUMS logic at least (we turn off GPG validation though, # see above) - new_version "$sector_size" v5 + new_version "$sector_size" v7 cat >"$CONFIGDIR/02-second.conf" <