lib/checkout: fallback to checksum for UNION_IDENTICAL

There's a subtle issue going on with the way we use `UNION_IDENTICAL`
now in rpm-ostree. Basically, the crux of the issue is that we checkout
the whole tree from the system repo, but then overlay packages by
checking out from the pkgcache repo. This is an easy way to break the
assumption that we will be merging hardlinks from the same repo.

This ends up causing issues like:
https://github.com/projectatomic/rpm-ostree/issues/1047

There, `vim-minimal` is already part of the host and has an object for
`/usr/share/man/man1/ex.1.gz`. `vim-common` has that same file, but
because it's unpacked in the pkgcache repo first, the hardlinks are not
the same.

There are a few ways we *could* work around this in rpm-ostree itself,
e.g. by re-establishing hardlinks when we do the content pull into the
system repo, but it still felt somewhat hacky. Let's just do this the
proper way and fall back to checksumming the target file if needed,
which is what librpm does as well in this case. Note that we only
checksum if they're not hard links, but they're the same size.

Closes: #1258
Approved by: cgwalters

diff --git a/src/libostree/ostree-core-private.h b/src/libostree/ostree-core-private.h
index d7677f38b00a9a533139a82b6d28cab4778cd0d3..d8b93b29e90f50d5b0b91b5dc8e00f4e6b2106b4 100644 (file)
--- a/src/libostree/ostree-core-private.h
+++ b/src/libostree/ostree-core-private.h
@@ -83,6 +83,7 @@ _ostree_make_temporary_symlink_at (int             tmp_dirfd,
 
 GFileInfo * _ostree_stbuf_to_gfileinfo (const struct stat *stbuf);
 gboolean _ostree_gfileinfo_equal (GFileInfo *a, GFileInfo *b);
+gboolean _ostree_stbuf_equal (struct stat *stbuf_a, struct stat *stbuf_b);
 GFileInfo * _ostree_mode_uidgid_to_gfileinfo (mode_t mode, uid_t uid, gid_t gid);
 
 static inline void

diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c
index 615f5dec71a84feb2d2566e937e4bc4ecf6311e9..542c54bf9e2599fab8442b730afd0cfe3dfd65ee 100644 (file)
--- a/src/libostree/ostree-core.c
+++ b/src/libostree/ostree-core.c
@@ -1576,6 +1576,24 @@ _ostree_gfileinfo_equal (GFileInfo *a, GFileInfo *b)
   return TRUE;
 }
 
+/* Same motives as _ostree_gfileinfo_equal(), but for stat structs. */
+gboolean
+_ostree_stbuf_equal (struct stat *stbuf_a, struct stat *stbuf_b)
+{
+  /* trivial case */
+  if (stbuf_a == stbuf_b)
+    return TRUE;
+  if (stbuf_a->st_mode != stbuf_b->st_mode)
+    return FALSE;
+  if (S_ISREG (stbuf_a->st_mode) && (stbuf_a->st_size != stbuf_b->st_size))
+    return FALSE;
+  if (stbuf_a->st_uid != stbuf_b->st_uid)
+    return FALSE;
+  if (stbuf_a->st_gid != stbuf_b->st_gid)
+    return FALSE;
+  return TRUE;
+}
+
 /* Many parts of libostree only care about mode,uid,gid - this creates
  * a new GFileInfo with those fields see.
  */

diff --git a/src/libostree/ostree-repo-checkout.c b/src/libostree/ostree-repo-checkout.c
index 99896142e6c1f0a36ab7b124f01b325683a80b14..d144d03e6f7017967f93476996675d366b2e1068 100644 (file)
--- a/src/libostree/ostree-repo-checkout.c
+++ b/src/libostree/ostree-repo-checkout.c
@@ -374,7 +374,8 @@ hardlink_add_tmp_name (OstreeRepo              *self,
 
 static gboolean
 checkout_file_hardlink (OstreeRepo                          *self,
-                        OstreeRepoCheckoutAtOptions           *options,
+                        const char                          *checksum,
+                        OstreeRepoCheckoutAtOptions         *options,
                         const char                          *loose_path,
                         int                                  destination_dfd,
                         const char                          *destination_name,
@@ -436,10 +437,28 @@ checkout_file_hardlink (OstreeRepo                          *self,
             if (!glnx_fstatat (destination_dfd, destination_name, &dest_stbuf,
                                AT_SYMLINK_NOFOLLOW, error))
               return FALSE;
-            const gboolean is_identical =
+            gboolean is_identical =
               (src_stbuf.st_dev == dest_stbuf.st_dev &&
                src_stbuf.st_ino == dest_stbuf.st_ino);
+            if (!is_identical && (_ostree_stbuf_equal (&src_stbuf, &dest_stbuf)))
+              {
+                /* As a last resort, do a checksum comparison. This is the case currently
+                 * with rpm-ostree pkg layering where we overlay from the pkgcache repo onto
+                 * a tree checked out from the system repo. Once those are united, we
+                 * shouldn't hit this anymore. https://github.com/ostreedev/ostree/pull/1258
+                 * */
+                OstreeChecksumFlags flags = 0;
+                if (self->disable_xattrs)
+                    flags |= OSTREE_CHECKSUM_FLAGS_IGNORE_XATTRS;
+
+                g_autofree char *actual_checksum = NULL;
+                if (!ostree_checksum_file_at (destination_dfd, destination_name,
+                                              &dest_stbuf, OSTREE_OBJECT_TYPE_FILE,
+                                              flags, &actual_checksum, cancellable, error))
+                  return FALSE;
 
+                is_identical = g_str_equal (checksum, actual_checksum);
+              }
             if (is_identical)
               ret_result = HARDLINK_RESULT_SKIP_EXISTED;
             else if (options->overwrite_mode == OSTREE_REPO_CHECKOUT_OVERWRITE_UNION_FILES)
@@ -563,6 +582,7 @@ checkout_one_file_at (OstreeRepo                        *repo,
                  the cache, which is in "bare" form */
               _ostree_loose_path (loose_path_buf, checksum, OSTREE_OBJECT_TYPE_FILE, OSTREE_REPO_MODE_BARE);
               if (!checkout_file_hardlink (current_repo,
+                                           checksum,
                                            options,
                                            loose_path_buf,
                                            destination_dfd, destination_name,
@@ -652,7 +672,7 @@ checkout_one_file_at (OstreeRepo                        *repo,
       }
       g_mutex_unlock (&repo->cache_lock);
 
-      if (!checkout_file_hardlink (repo, options, loose_path_buf,
+      if (!checkout_file_hardlink (repo, checksum, options, loose_path_buf,
                                    destination_dfd, destination_name,
                                    FALSE, &hardlink_res,
                                    cancellable, error))

diff --git a/tests/basic-test.sh b/tests/basic-test.sh
index 3abefd102c8af36e120a2545b7ae5b5a82c2ac2a..9c22fe349a58d844d407ad3f3bacff001c4a3b13 100644 (file)
--- a/tests/basic-test.sh
+++ b/tests/basic-test.sh
@@ -546,6 +546,12 @@ for x in $(seq 3); do
         assert_file_has_content union-identical-test/usr/share/licenses/${v} GPL
     done
 done
+# now checkout the first pkg in force copy mode to make sure we can checksum
+rm union-identical-test -rf
+$OSTREE checkout --force-copy union-identical-pkg1 union-identical-test
+for x in 2 3; do
+    $OSTREE checkout ${CHECKOUT_H_ARGS} --union-identical union-identical-pkg${x} union-identical-test
+done
 echo "ok checkout union identical merges"
 
 # Make conflicting packages, one with regfile, one with symlink