sysupdate: Evaluate all patterns before descending into a subdir

When we have two match patterns, e.g., because the repository layout
changed and we look for old and new style files, we can have the case
that one pattern suggests a descend into a subdir but the other pattern
would be a direct match. This was missed and only the descent was done.
Yet the other way round also has to be covered: We can't just use the
direct match because it might be that it's rejected if the type doesn't
fit (directory vs. regular file). In this case we should still descent.

Remember that we want to descend but continue checking the other
pattern first to maybe get a direct match. Introduce a new combined
return code YES_AND_RETRY to give the full information to the caller
which now can decide to fallback to a descent. While at it, use the
stat info for the directory check. Also add tests to ensure that the
order of patterns doesn't matter and we handle the above corner cases.

diff --git a/src/sysupdate/sysupdate-cleanup.c b/src/sysupdate/sysupdate-cleanup.c
index 2d0229423d5bd4b3a55631927e3cf70ec4c6f0d5..f45ac37e2c9726e7d27c98b3fd757fa0b112385c 100644 (file)
--- a/src/sysupdate/sysupdate-cleanup.c
+++ b/src/sysupdate/sysupdate-cleanup.c
@@ -184,7 +184,7 @@ static int context_is_path_currently_owned(
                         return log_error_errno(r, "Failed to match patterns '%s' against '%s': %m", cl, relpath);
                 }
 
-                if (IN_SET(r, PATTERN_MATCH_YES, PATTERN_MATCH_RETRY)) /* Yay, this path is pinned by this transfer file */
+                if (IN_SET(r, PATTERN_MATCH_YES, PATTERN_MATCH_RETRY, PATTERN_MATCH_YES_AND_RETRY)) /* Yay, this path is pinned by this transfer file */
                         return true;
 
                 assert(r == PATTERN_MATCH_NO);

diff --git a/src/sysupdate/sysupdate-pattern.c b/src/sysupdate/sysupdate-pattern.c
index ddc9adb881ed1c8445ec59f80c263d3acd05e8ee..459c8fda29d5118508e1aae8d91d2faef3e1d155 100644 (file)
--- a/src/sysupdate/sysupdate-pattern.c
+++ b/src/sysupdate/sysupdate-pattern.c
@@ -454,12 +454,20 @@ retry:
 }
 
 int pattern_match_many(char **patterns, const char *s, InstanceMetadata *ret) {
-        _cleanup_(instance_metadata_destroy) InstanceMetadata found = INSTANCE_METADATA_NULL;
+        _cleanup_(instance_metadata_destroy) InstanceMetadata matched = INSTANCE_METADATA_NULL;
+        bool have_match = false, retry_descent = false;
         int r;
 
+        /* Evaluate all patterns and report the combined result: a direct match wins and provides the
+         * extracted fields, but when another pattern would descend into a subdirectory this is reported
+         * too, so the caller can descend as fallback when the direct match is no valid candidate. */
         STRV_FOREACH(p, patterns) {
+                _cleanup_(instance_metadata_destroy) InstanceMetadata found = INSTANCE_METADATA_NULL;
                 const char *pat = *p, *input_path;
 
+                if (have_match && retry_descent)
+                        break; /* nothing more to learn */
+
                 /* A glob directory prefix on a pattern means to match the rest of the pattern against the
                  * last path component only (the basename, "find this file anywhere under the source tree") */
                 if (pattern_skip_glob_directory_prefix(&pat))
@@ -470,20 +478,20 @@ int pattern_match_many(char **patterns, const char *s, InstanceMetadata *ret) {
                 r = pattern_match(pat, input_path, &found);
                 if (r < 0)
                         return r;
-                if (r != PATTERN_MATCH_NO) {
-                        if (ret) {
-                                *ret = found;
-                                found = (InstanceMetadata) INSTANCE_METADATA_NULL;
-                        }
-
-                        return r;
+                if (r == PATTERN_MATCH_YES && !have_match) {
+                        matched = TAKE_GENERIC(found, InstanceMetadata, INSTANCE_METADATA_NULL);
+                        have_match = true;
                 }
+                if (r == PATTERN_MATCH_RETRY)
+                        retry_descent = true;
         }
 
         if (ret)
-                *ret = (InstanceMetadata) INSTANCE_METADATA_NULL;
+                *ret = TAKE_GENERIC(matched, InstanceMetadata, INSTANCE_METADATA_NULL);
 
-        return PATTERN_MATCH_NO;
+        if (have_match)
+                return retry_descent ? PATTERN_MATCH_YES_AND_RETRY : PATTERN_MATCH_YES;
+        return retry_descent ? PATTERN_MATCH_RETRY : PATTERN_MATCH_NO;
 }
 
 bool pattern_skip_glob_directory_prefix(const char **pattern) {

diff --git a/src/sysupdate/sysupdate-pattern.h b/src/sysupdate/sysupdate-pattern.h
index 8bc9f09e86182287906e0e3f0c34a911711d6e84..6bc9b243c1368f53645bf4c7b681bdd0ff7ec3a3 100644 (file)
--- a/src/sysupdate/sysupdate-pattern.h
+++ b/src/sysupdate/sysupdate-pattern.h
@@ -7,6 +7,7 @@ enum {
         PATTERN_MATCH_NO,
         PATTERN_MATCH_YES,
         PATTERN_MATCH_RETRY,
+        PATTERN_MATCH_YES_AND_RETRY,
 };
 
 int pattern_match(const char *pattern, const char *s, InstanceMetadata *ret);

diff --git a/src/sysupdate/sysupdate-resource.c b/src/sysupdate/sysupdate-resource.c
index ab98f649eb5d7c482a0f8b1a5dbb58300f8bda58..08a3f9ef7cafe2cda629eff174bf5b60786855a2 100644 (file)
--- a/src/sysupdate/sysupdate-resource.c
+++ b/src/sysupdate/sysupdate-resource.c
@@ -185,7 +185,10 @@ static int resource_load_from_directory_recursive(
                                 if (!S_ISDIR(st.st_mode)) /* The pattern continues with '/' but this is no directory */
                                         continue;
                                 descend = true;
-                        }
+                        } else if (r == PATTERN_MATCH_YES_AND_RETRY && S_ISDIR(st.st_mode) && m != S_IFDIR)
+                                /* The direct match is unusable since a directory can't be an instance here,
+                                 * but another pattern wants to descend, so do that instead */
+                                descend = true;
                 }
 
                 if (descend) {
@@ -205,7 +208,7 @@ static int resource_load_from_directory_recursive(
                         continue;
                 }
 
-                if (de->d_type == DT_DIR && m != S_IFDIR)
+                if (S_ISDIR(st.st_mode) && m != S_IFDIR)
                         continue;
 
                 joined = path_join(rr->path, rel_joined);
@@ -610,7 +613,7 @@ static int resource_load_from_web(
                         r = pattern_match_many(rr->patterns, fn, &extracted_fields);
                         if (r < 0)
                                 return log_error_errno(r, "Failed to match pattern: %m");
-                        if (r == PATTERN_MATCH_YES) {
+                        if (IN_SET(r, PATTERN_MATCH_YES, PATTERN_MATCH_YES_AND_RETRY)) {
                                 _cleanup_free_ char *path = NULL;
 
                                 r = import_url_append_component(rr->path, fn, &path);

diff --git a/test/units/TEST-72-SYSUPDATE.sh b/test/units/TEST-72-SYSUPDATE.sh
index 5c812420d02b7573d564a0529c06444e05dcf042..d3a8a4bc61ff96bedd804b2af41f2ba3d397492f 100755 (executable)
--- a/test/units/TEST-72-SYSUPDATE.sh
+++ b/test/units/TEST-72-SYSUPDATE.sh
@@ -1326,4 +1326,104 @@ EOF
 cmp "$WORKDIR/source/depth-v3/contents/image.raw" "$WORKDIR/blobs/depth-v3.raw"
 rm -rf "$CONFIGDIR/01-depth.transfer" "$WORKDIR/source/depth-v3"
 
+# Pattern matching should not depend on the pattern order. A transfer can list one pattern per repository
+# layout, e.g., when a source switches from one directory per version (bundle-v1/image.raw) to flat files
+# (bundle-v2.raw) and both layouts coexist during a migration.
+# Check that after a subdirectory pattern which descends we still can match a top-level file through a
+# '**/' pattern.
+# Also check that after a non-matching '**/' pattern we still can match a plain top-level file.
+# Then check that after a subdirectory pattern we can still match a plain top-level file.
+# The first and last work because the wildcard field also captures the '.raw' suffix, so the top-level file
+# matches the directory component of the subdirectory pattern and triggers the descend-retry logic.
+# The fourth transfer checks the vice versa migration where one directory per version is the new layout and
+# the old versions are flat files without a suffix: the directory name fully matches the flat pattern but
+# must still be descended into for the subdirectory pattern.
+rm -rf "$CONFIGDIR" "$WORKDIR/blobs" "$WORKDIR/source/bundle-v1" "$WORKDIR/source/pack-v2"
+mkdir -p "$CONFIGDIR" "$WORKDIR/blobs"/{glob-last,glob-first,subdir-first,yes-and-retry} \
+         "$WORKDIR/source/bundle-v1" "$WORKDIR/source/pack-v2"
+echo 'version-one' >"$WORKDIR/source/bundle-v1/image.raw"
+echo 'version-two' >"$WORKDIR/source/bundle-v2.raw"
+echo 'version-v1' >"$WORKDIR/source/pack-v1"
+echo 'version-v2' >"$WORKDIR/source/pack-v2/image.raw"
+cat >"$CONFIGDIR/01-glob-last.transfer" <<EOF
+[Source]
+Type=regular-file
+Path=$WORKDIR/source
+MatchPattern=bundle-@v/image.raw **/bundle-@v.raw
+
+[Target]
+Type=regular-file
+Path=$WORKDIR/blobs/glob-last
+MatchPattern=bundle-@v.raw
+InstancesMax=1
+EOF
+cat >"$CONFIGDIR/02-glob-first.transfer" <<EOF
+[Source]
+Type=regular-file
+Path=$WORKDIR/source
+MatchPattern=**/bundle-@v.img bundle-@v.raw
+
+[Target]
+Type=regular-file
+Path=$WORKDIR/blobs/glob-first
+MatchPattern=bundle-@v.raw
+InstancesMax=1
+EOF
+cat >"$CONFIGDIR/03-subdir-first.transfer" <<EOF
+[Source]
+Type=regular-file
+Path=$WORKDIR/source
+MatchPattern=bundle-@v/image.raw bundle-@v.raw
+
+[Target]
+Type=regular-file
+Path=$WORKDIR/blobs/subdir-first
+MatchPattern=bundle-@v.raw
+InstancesMax=1
+EOF
+cat >"$CONFIGDIR/04-yes-and-retry.transfer" <<EOF
+[Source]
+Type=regular-file
+Path=$WORKDIR/source
+MatchPattern=pack-@v/image.raw pack-@v
+
+[Target]
+Type=regular-file
+Path=$WORKDIR/blobs/yes-and-retry
+MatchPattern=pack-@v
+InstancesMax=1
+EOF
+"$SYSUPDATE" --verify=no update
+for target in glob-last glob-first subdir-first; do
+    cmp "$WORKDIR/source/bundle-v2.raw" "$WORKDIR/blobs/$target/bundle-v2.raw"
+done
+cmp "$WORKDIR/source/pack-v2/image.raw" "$WORKDIR/blobs/yes-and-retry/pack-v2"
+rm -rf "$CONFIGDIR"/{01-glob-last,02-glob-first,03-subdir-first,04-yes-and-retry}.transfer \
+       "$WORKDIR/source/bundle-v2.raw" "$WORKDIR/source/bundle-v1" \
+       "$WORKDIR/source/pack-v1" "$WORKDIR/source/pack-v2"
+
+# A manifest entry that both directly matches a flat pattern and prefix-matches a subdirectory pattern
+# must be downloaded, here the flat file is the newest version (this exercises YES_AND_RETRY)
+rm -rf "$CONFIGDIR" "$WORKDIR/blobs" "$WORKDIR/source/pack-v1"
+mkdir -p "$CONFIGDIR" "$WORKDIR/blobs" "$WORKDIR/source/pack-v1"
+echo 'version-v1' >"$WORKDIR/source/pack-v1/image.raw"
+echo 'version-v2' >"$WORKDIR/source/pack-v2"
+(cd "$WORKDIR/source" && sha256sum pack-v1/image.raw pack-v2 >SHA256SUMS)
+cat >"$CONFIGDIR/01-manifest-yes-and-retry.transfer" <<EOF
+[Source]
+Type=url-file
+Path=file://$WORKDIR/source
+MatchPattern=pack-@v/image.raw pack-@v
+
+[Target]
+Type=regular-file
+Path=$WORKDIR/blobs
+MatchPattern=pack-@v
+InstancesMax=1
+EOF
+"$SYSUPDATE" --verify=no update
+cmp "$WORKDIR/source/pack-v2" "$WORKDIR/blobs/pack-v2"
+rm -rf "$CONFIGDIR/01-manifest-yes-and-retry.transfer" \
+       "$WORKDIR/source/pack-v1" "$WORKDIR/source/pack-v2"
+
 touch /testok