Skip to content

Commit

Permalink
lib/checkout: fallback to checksum for UNION_IDENTICAL
Browse files Browse the repository at this point in the history
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:
coreos/rpm-ostree#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.
  • Loading branch information
jlebon committed Oct 10, 2017
1 parent a0cd30d commit 56bdefb
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 3 deletions.
23 changes: 20 additions & 3 deletions src/libostree/ostree-repo-checkout.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -436,10 +437,25 @@ 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 && (src_stbuf.st_size == dest_stbuf.st_size))
{
/* resort to a checksum check */
g_autofree char *abspath =
glnx_fdrel_abspath (destination_dfd, destination_name);
g_autoptr(GFile) dest_file = g_file_new_for_path (abspath);

/* XXX: need an fd-relative version of this API */
g_autofree guchar *csum = NULL;
if (!ostree_checksum_file (dest_file, OSTREE_OBJECT_TYPE_FILE,
&csum, cancellable, error))
return FALSE;

g_autofree char *actual_checksum = ostree_checksum_from_bytes (csum);
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)
Expand Down Expand Up @@ -563,6 +579,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,
Expand Down Expand Up @@ -652,7 +669,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))
Expand Down
6 changes: 6 additions & 0 deletions tests/basic-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,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-pkg${x} 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
Expand Down

0 comments on commit 56bdefb

Please sign in to comment.