diff --git a/src/libostree/ostree-core-private.h b/src/libostree/ostree-core-private.h index 783eacd33a..0658a0cbbd 100644 --- a/src/libostree/ostree-core-private.h +++ b/src/libostree/ostree-core-private.h @@ -89,6 +89,7 @@ _ostree_make_temporary_symlink_at (int tmp_dirfd, GError **error); GFileInfo * _ostree_stbuf_to_gfileinfo (const struct stat *stbuf); +gboolean _ostree_gfileinfo_equal (GFileInfo *a, GFileInfo *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 5a4b58f46d..08c2892484 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -1568,12 +1568,52 @@ _ostree_stbuf_to_gfileinfo (const struct stat *stbuf) g_file_info_set_attribute_uint32 (ret, "unix::uid", stbuf->st_uid); g_file_info_set_attribute_uint32 (ret, "unix::gid", stbuf->st_gid); g_file_info_set_attribute_uint32 (ret, "unix::mode", mode); + + /* those aren't stored by ostree, but used by the devino cache */ + g_file_info_set_attribute_uint32 (ret, "unix::device", stbuf->st_dev); + g_file_info_set_attribute_uint64 (ret, "unix::inode", stbuf->st_ino); + if (S_ISREG (mode)) g_file_info_set_attribute_uint64 (ret, "standard::size", stbuf->st_size); return ret; } +/** + * _ostree_gfileinfo_equal: + * @a: First file info + * @b: Second file info + * + * OSTree only cares about a subset of file attributes. This function + * checks whether two #GFileInfo objects are equal as far as OSTree is + * concerned. + * + * Returns: TRUE if the #GFileInfo objects are OSTree-equivalent. + */ +gboolean +_ostree_gfileinfo_equal (GFileInfo *a, GFileInfo *b) +{ + /* trivial case */ + if (a == b) + return TRUE; + +#define CHECK_ONE_ATTR(type, attr, a, b) \ + do { if (g_file_info_get_attribute_##type(a, attr) != \ + g_file_info_get_attribute_##type(b, attr)) \ + return FALSE; \ + } while (0) + + CHECK_ONE_ATTR (uint32, "unix::uid", a, b); + CHECK_ONE_ATTR (uint32, "unix::gid", a, b); + CHECK_ONE_ATTR (uint32, "unix::mode", a, b); + CHECK_ONE_ATTR (uint32, "standard::type", a, b); + CHECK_ONE_ATTR (uint64, "standard::size", a, b); + +#undef CHECK_ONE_ATTR + + return TRUE; +} + GFileInfo * _ostree_mode_uidgid_to_gfileinfo (mode_t mode, uid_t uid, gid_t gid) { diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 1b7d380cf0..c4484f44b9 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2363,58 +2363,68 @@ ptrarray_path_join (GPtrArray *path) } static gboolean -get_modified_xattrs (OstreeRepo *self, - OstreeRepoCommitModifier *modifier, - const char *relpath, - GFileInfo *file_info, - GFile *path, - int dfd, - const char *dfd_subpath, - GVariant **out_xattrs, - GCancellable *cancellable, - GError **error) -{ - g_autoptr(GVariant) ret_xattrs = NULL; - - if (modifier && modifier->xattr_callback) - { - ret_xattrs = modifier->xattr_callback (self, relpath, file_info, - modifier->xattr_user_data); - } - else if (!(modifier && (modifier->flags & (OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS | - OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS)) > 0) - && !self->disable_xattrs) +get_final_xattrs (OstreeRepo *self, + OstreeRepoCommitModifier *modifier, + const char *relpath, + GFileInfo *file_info, + GFile *path, + int dfd, + const char *dfd_subpath, + GVariant **out_xattrs, + gboolean *out_modified, + GCancellable *cancellable, + GError **error) +{ + /* track whether the returned xattrs differ from the file on disk */ + gboolean modified = TRUE; + const gboolean skip_xattrs = (modifier && + modifier->flags & (OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS | + OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS)) > 0; + + /* fetch on-disk xattrs if needed & not disabled */ + g_autoptr(GVariant) original_xattrs = NULL; + if (!skip_xattrs && !self->disable_xattrs) { if (path && OSTREE_IS_REPO_FILE (path)) { - if (!ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (path), - &ret_xattrs, - cancellable, - error)) + if (!ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (path), &original_xattrs, + cancellable, error)) return FALSE; } else if (path) { if (!glnx_dfd_name_get_all_xattrs (AT_FDCWD, gs_file_get_path_cached (path), - &ret_xattrs, cancellable, error)) + &original_xattrs, cancellable, error)) return FALSE; } else if (dfd_subpath == NULL) { g_assert (dfd != -1); - if (!glnx_fd_get_all_xattrs (dfd, &ret_xattrs, - cancellable, error)) + if (!glnx_fd_get_all_xattrs (dfd, &original_xattrs, cancellable, error)) return FALSE; } else { g_assert (dfd != -1); - if (!glnx_dfd_name_get_all_xattrs (dfd, dfd_subpath, &ret_xattrs, - cancellable, error)) + if (!glnx_dfd_name_get_all_xattrs (dfd, dfd_subpath, &original_xattrs, + cancellable, error)) return FALSE; } + + g_assert (original_xattrs); + } + + g_autoptr(GVariant) ret_xattrs = NULL; + if (modifier && modifier->xattr_callback) + { + ret_xattrs = modifier->xattr_callback (self, relpath, file_info, + modifier->xattr_user_data); } + /* if callback returned NULL or didn't exist, default to on-disk state */ + if (!ret_xattrs && original_xattrs) + ret_xattrs = g_variant_ref (original_xattrs); + if (modifier && modifier->sepolicy) { g_autofree char *label = NULL; @@ -2436,10 +2446,9 @@ get_modified_xattrs (OstreeRepo *self, { /* drop out any existing SELinux policy from the set, so we don't end up * counting it twice in the checksum */ - g_autoptr(GVariant) new_ret_xattrs = NULL; - new_ret_xattrs = _ostree_filter_selinux_xattr (ret_xattrs); + GVariant* new_ret_xattrs = _ostree_filter_selinux_xattr (ret_xattrs); g_variant_unref (ret_xattrs); - ret_xattrs = g_steal_pointer (&new_ret_xattrs); + ret_xattrs = new_ret_xattrs; } /* ret_xattrs may be NULL */ @@ -2458,8 +2467,13 @@ get_modified_xattrs (OstreeRepo *self, } } + if (original_xattrs && ret_xattrs && g_variant_equal (original_xattrs, ret_xattrs)) + modified = FALSE; + if (out_xattrs) *out_xattrs = g_steal_pointer (&ret_xattrs); + if (out_modified) + *out_modified = modified; return TRUE; } @@ -2506,6 +2520,7 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, g_autoptr(GFileInfo) modified_info = NULL; OstreeRepoCommitFilterResult filter_result = _ostree_repo_commit_modifier_apply (self, modifier, child_relpath, child_info, &modified_info); + const gboolean child_info_was_modified = !_ostree_gfileinfo_equal (child_info, modified_info); if (filter_result != OSTREE_REPO_COMMIT_FILTER_ALLOW) { @@ -2567,16 +2582,26 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, else { guint64 file_obj_length; - const char *loose_checksum; g_autoptr(GInputStream) file_input = NULL; - g_autoptr(GVariant) xattrs = NULL; g_autoptr(GInputStream) file_object_input = NULL; g_autofree guchar *child_file_csum = NULL; g_autofree char *tmp_checksum = NULL; - loose_checksum = devino_cache_lookup (self, modifier, - g_file_info_get_attribute_uint32 (child_info, "unix::device"), - g_file_info_get_attribute_uint64 (child_info, "unix::inode")); + g_autoptr(GVariant) xattrs = NULL; + gboolean xattrs_were_modified; + if (!get_final_xattrs (self, modifier, child_relpath, child_info, child, + dfd_iter != NULL ? dfd_iter->fd : -1, name, &xattrs, + &xattrs_were_modified, cancellable, error)) + return FALSE; + + /* only check the devino cache if the file info & xattrs were not modified */ + const char *loose_checksum = NULL; + if (!child_info_was_modified && !xattrs_were_modified) + { + guint32 dev = g_file_info_get_attribute_uint32 (child_info, "unix::device"); + guint64 inode = g_file_info_get_attribute_uint64 (child_info, "unix::inode"); + loose_checksum = devino_cache_lookup (self, modifier, dev, inode); + } if (loose_checksum) { @@ -2602,12 +2627,6 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, } } - if (!get_modified_xattrs (self, modifier, - child_relpath, child_info, child, dfd_iter != NULL ? dfd_iter->fd : -1, name, - &xattrs, - cancellable, error)) - return FALSE; - if (!ostree_raw_file_to_content_stream (file_input, modified_info, xattrs, &file_object_input, &file_obj_length, @@ -2681,10 +2700,8 @@ write_directory_to_mtree_internal (OstreeRepo *self, if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW) { - if (!get_modified_xattrs (self, modifier, relpath, child_info, - dir, -1, NULL, - &xattrs, - cancellable, error)) + if (!get_final_xattrs (self, modifier, relpath, child_info, dir, -1, NULL, + &xattrs, NULL, cancellable, error)) return FALSE; g_autofree guchar *child_file_csum = NULL; @@ -2768,10 +2785,8 @@ write_dfd_iter_to_mtree_internal (OstreeRepo *self, if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW) { - if (!get_modified_xattrs (self, modifier, relpath, modified_info, - NULL, src_dfd_iter->fd, NULL, - &xattrs, - cancellable, error)) + if (!get_final_xattrs (self, modifier, relpath, modified_info, NULL, src_dfd_iter->fd, + NULL, &xattrs, NULL, cancellable, error)) return FALSE; if (!_ostree_repo_write_directory_meta (self, modified_info, xattrs, &child_file_csum, @@ -2801,16 +2816,6 @@ write_dfd_iter_to_mtree_internal (OstreeRepo *self, if (!glnx_fstatat (src_dfd_iter->fd, dent->d_name, &stbuf, AT_SYMLINK_NOFOLLOW, error)) return FALSE; - const char *loose_checksum = devino_cache_lookup (self, modifier, stbuf.st_dev, stbuf.st_ino); - if (loose_checksum) - { - if (!ostree_mutable_tree_replace_file (mtree, dent->d_name, loose_checksum, - error)) - return FALSE; - - continue; - } - g_autoptr(GFileInfo) child_info = _ostree_stbuf_to_gfileinfo (&stbuf); g_file_info_set_name (child_info, dent->d_name); diff --git a/tests/basic-test.sh b/tests/basic-test.sh index 4df6a0798f..a01f437aa1 100644 --- a/tests/basic-test.sh +++ b/tests/basic-test.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..$((72 + ${extra_basic_tests:-0}))" +echo "1..$((73 + ${extra_basic_tests:-0}))" CHECKOUT_U_ARG="" CHECKOUT_H_ARGS="-H" @@ -602,6 +602,24 @@ $OSTREE checkout test2 test2-checkout (cd test2-checkout && $OSTREE commit ${COMMIT_ARGS} --link-checkout-speedup -b test2 -s "tmp") echo "ok commit with link speedup" +cd ${test_tmpdir} +rm -rf test2-checkout +$OSTREE checkout test2 test2-checkout +# set cow to different perms, but re-set cowro to the same perms +cat > statoverride.txt < stats.txt +$OSTREE diff test2 test2-tmp > diff-test2 +assert_file_has_content diff-test2 'M */baz/cow$' +assert_not_file_has_content diff-test2 'M */baz/cowro$' +assert_not_file_has_content diff-test2 'baz/saucer' +# only /baz/cow is a cache miss +assert_file_has_content stats.txt '^Content Written: 1$' +echo "ok commit with link speedup and modifier" + cd ${test_tmpdir} $OSTREE ls test2 echo "ok ls with no argument" diff --git a/tests/test-basic-c.c b/tests/test-basic-c.c index 27b9744a38..163774f326 100644 --- a/tests/test-basic-c.c +++ b/tests/test-basic-c.c @@ -236,6 +236,125 @@ test_object_writes (gconstpointer data) } } +static GVariant* +xattr_cb (OstreeRepo *repo, + const char *path, + GFileInfo *file_info, + gpointer user_data) +{ + GVariant *xattr = user_data; + if (g_str_equal (path, "/baz/cow")) + return g_variant_ref (xattr); + return NULL; +} + +/* check that using a devino cache doesn't cause us to ignore xattr callbacks */ +static void +test_devino_cache_xattrs (void) +{ + g_autoptr(GError) error = NULL; + gboolean ret = FALSE; + + g_autoptr(GFile) repo_path = g_file_new_for_path ("repo"); + + /* re-initialize as bare */ + ret = ot_test_run_libtest ("setup_test_repository bare", &error); + g_assert_no_error (error); + g_assert (ret); + + gboolean on_overlay; + ret = ot_check_for_overlay (&on_overlay, &error); + g_assert_no_error (error); + g_assert (ret); + + if (on_overlay) + { + g_test_skip ("overlayfs detected"); + return; + } + + g_autoptr(OstreeRepo) repo = ostree_repo_new (repo_path); + ret = ostree_repo_open (repo, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + g_autofree char *csum = NULL; + ret = ostree_repo_resolve_rev (repo, "test2", FALSE, &csum, &error); + g_assert_no_error (error); + g_assert (ret); + + g_autoptr(OstreeRepoDevInoCache) cache = ostree_repo_devino_cache_new (); + + OstreeRepoCheckoutAtOptions options = {0,}; + options.no_copy_fallback = TRUE; + options.devino_to_csum_cache = cache; + ret = ostree_repo_checkout_at (repo, &options, AT_FDCWD, "checkout", csum, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + g_autoptr(OstreeMutableTree) mtree = ostree_mutable_tree_new (); + g_autoptr(OstreeRepoCommitModifier) modifier = + ostree_repo_commit_modifier_new (0, NULL, NULL, NULL); + ostree_repo_commit_modifier_set_devino_cache (modifier, cache); + + g_auto(GVariantBuilder) builder; + g_variant_builder_init (&builder, (GVariantType*)"a(ayay)"); + g_variant_builder_add (&builder, "(@ay@ay)", + g_variant_new_bytestring ("user.myattr"), + g_variant_new_bytestring ("data")); + g_autoptr(GVariant) orig_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder)); + + ret = ostree_repo_prepare_transaction (repo, NULL, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + ostree_repo_commit_modifier_set_xattr_callback (modifier, xattr_cb, NULL, orig_xattrs); + ret = ostree_repo_write_dfd_to_mtree (repo, AT_FDCWD, "checkout", + mtree, modifier, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + g_autoptr(GFile) root = NULL; + ret = ostree_repo_write_mtree (repo, mtree, &root, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + /* now check that the final xattr matches */ + g_autoptr(GFile) baz_child = g_file_get_child (root, "baz"); + g_autoptr(GFile) cow_child = g_file_get_child (baz_child, "cow"); + + g_autoptr(GVariant) xattrs = NULL; + ret = ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (cow_child), &xattrs, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + gboolean found_xattr = FALSE; + gsize n = g_variant_n_children (xattrs); + for (gsize i = 0; i < n; i++) + { + const guint8* name; + const guint8* value; + g_variant_get_child (xattrs, i, "(^&ay^&ay)", &name, &value); + + if (g_str_equal ((const char*)name, "user.myattr")) + { + g_assert_cmpstr ((const char*)value, ==, "data"); + found_xattr = TRUE; + break; + } + } + + g_assert (found_xattr); + + OstreeRepoTransactionStats stats; + ret = ostree_repo_commit_transaction (repo, &stats, NULL, &error); + g_assert_no_error (error); + g_assert (ret); + + /* we should only have had to checksum /baz/cow */ + g_assert_cmpint (stats.content_objects_written, ==, 1); +} + int main (int argc, char **argv) { g_autoptr(GError) error = NULL; @@ -243,13 +362,14 @@ int main (int argc, char **argv) g_test_init (&argc, &argv, NULL); - repo = ot_test_setup_repo (NULL, &error); + repo = ot_test_setup_repo (NULL, &error); if (!repo) goto out; g_test_add_data_func ("/repo-not-system", repo, test_repo_is_not_system); g_test_add_data_func ("/raw-file-to-archive-stream", repo, test_raw_file_to_archive_stream); g_test_add_data_func ("/objectwrites", repo, test_object_writes); + g_test_add_func ("/xattrs-devino-cache", test_devino_cache_xattrs); g_test_add_func ("/remotename", test_validate_remotename); return g_test_run();