From 4dee1984dccace10572192d2f63b7a6fc7d453ee Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sat, 24 Jun 2017 09:30:02 -0400 Subject: [PATCH] lib: Hoist unlinkat() cleanup API to fsutil, use in pull The pull code also could make use of this in both the metadata and content paths. I changed it to own the tempfile malloc (just like `GLnxTmpFile`), since there's no reason to have different lifetimes for the filename and the file, and that way we only have one variable rather than two. The content path turns out to be a special case though, where at least for mirroring archives, we directly pass the file *path* down into `_ostree_repo_commit_loose_final()`. This is prep for `GLnxTmpFile` porting. Closes: #957 Approved by: jlebon --- src/libostree/ostree-repo-commit.c | 35 ++++++------------------------ src/libostree/ostree-repo-pull.c | 34 ++++++++++++++--------------- src/libotutil/ot-fs-utils.h | 28 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 46 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 37e3d7e5..32170ac7 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -567,23 +567,6 @@ create_regular_tmpfile_linkable_with_content (OstreeRepo *self, return TRUE; } -/* A little helper to call unlinkat() as a cleanup - * function. Mostly only necessary to handle - * deletion of temporary symlinks. - */ -typedef struct { - int dfd; - const char *path; -} CleanupUnlinkat; - -static void -cleanup_unlinkat (CleanupUnlinkat *cleanup) -{ - if (cleanup->path) - (void) unlinkat (cleanup->dfd, cleanup->path, 0); -} -G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(CleanupUnlinkat, cleanup_unlinkat); - /* Write a content object. */ static gboolean write_content_object (OstreeRepo *self, @@ -651,9 +634,8 @@ write_content_object (OstreeRepo *self, * temp_filename might also be a symlink. Hence the CleanupUnlinkat * which handles that case. */ - g_auto(CleanupUnlinkat) tmp_unlinker = { self->tmp_dir_fd, NULL }; + g_auto(OtCleanupUnlinkat) tmp_unlinker = { self->tmp_dir_fd, NULL }; glnx_fd_close int temp_fd = -1; - g_autofree char *temp_filename = NULL; gssize unpacked_size = 0; gboolean indexable = FALSE; if ((_ostree_repo_mode_is_bare (repo_mode)) && !phys_object_is_symlink) @@ -661,10 +643,9 @@ write_content_object (OstreeRepo *self, guint64 size = g_file_info_get_size (file_info); if (!create_regular_tmpfile_linkable_with_content (self, size, file_input, - &temp_fd, &temp_filename, + &temp_fd, &tmp_unlinker.path, cancellable, error)) return FALSE; - tmp_unlinker.path = temp_filename; } else if (_ostree_repo_mode_is_bare (repo_mode) && phys_object_is_symlink) { @@ -672,10 +653,9 @@ write_content_object (OstreeRepo *self, regular file and take the branch above */ if (!_ostree_make_temporary_symlink_at (self->tmp_dir_fd, g_file_info_get_symlink_target (file_info), - &temp_filename, + &tmp_unlinker.path, cancellable, error)) return FALSE; - tmp_unlinker.path = temp_filename; } else if (repo_mode == OSTREE_REPO_MODE_ARCHIVE_Z2) { @@ -688,10 +668,9 @@ write_content_object (OstreeRepo *self, indexable = TRUE; if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, - &temp_fd, &temp_filename, + &temp_fd, &tmp_unlinker.path, error)) return FALSE; - tmp_unlinker.path = temp_filename; temp_out = g_unix_output_stream_new (temp_fd, FALSE); file_meta = _ostree_zlib_file_header_new (file_info, xattrs); @@ -758,15 +737,15 @@ write_content_object (OstreeRepo *self, const guint32 gid = g_file_info_get_attribute_uint32 (file_info, "unix::gid"); const guint32 mode = g_file_info_get_attribute_uint32 (file_info, "unix::mode"); if (!commit_loose_content_object (self, actual_checksum, - temp_filename, + tmp_unlinker.path, object_file_type == G_FILE_TYPE_SYMBOLIC_LINK, uid, gid, mode, xattrs, temp_fd, cancellable, error)) return glnx_prefix_error (error, "Writing object %s.%s", actual_checksum, ostree_object_type_to_string (OSTREE_OBJECT_TYPE_FILE)); - /* Clear the unlinker path, it was consumed */ - tmp_unlinker.path = NULL; + /* Clear the unlinker, it was consumed */ + ot_cleanup_unlinkat_clear (&tmp_unlinker); /* Update size metadata if configured */ if (indexable && object_file_type == G_FILE_TYPE_REGULAR) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 4c87199a..edfd8d4f 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -910,13 +910,13 @@ content_fetch_on_complete (GObject *object, g_autoptr(GVariant) xattrs = NULL; g_autoptr(GInputStream) file_in = NULL; g_autoptr(GInputStream) object_input = NULL; - g_autofree char *temp_path = NULL; + g_auto(OtCleanupUnlinkat) tmp_unlinker = { _ostree_fetcher_get_dfd (fetcher), NULL }; const char *checksum; g_autofree char *checksum_obj = NULL; OstreeObjectType objtype; gboolean free_fetch_data = TRUE; - if (!_ostree_fetcher_request_to_tmpfile_finish (fetcher, result, &temp_path, error)) + if (!_ostree_fetcher_request_to_tmpfile_finish (fetcher, result, &tmp_unlinker.path, error)) goto out; ostree_object_name_deserialize (fetch_data->object, &checksum, &objtype); @@ -941,9 +941,11 @@ content_fetch_on_complete (GObject *object, if (!have_object) { if (!_ostree_repo_commit_loose_final (pull_data->repo, checksum, OSTREE_OBJECT_TYPE_FILE, - _ostree_fetcher_get_dfd (fetcher), -1, temp_path, + tmp_unlinker.dfd, -1, tmp_unlinker.path, cancellable, error)) goto out; + /* The path was consumed */ + ot_cleanup_unlinkat_clear (&tmp_unlinker); } pull_data->n_fetched_content++; } @@ -952,20 +954,16 @@ content_fetch_on_complete (GObject *object, /* Non-mirroring path */ if (!ostree_content_file_parse_at (TRUE, _ostree_fetcher_get_dfd (fetcher), - temp_path, FALSE, + tmp_unlinker.path, FALSE, &file_in, &file_info, &xattrs, cancellable, error)) - { - /* If it appears corrupted, delete it */ - (void) unlinkat (_ostree_fetcher_get_dfd (fetcher), temp_path, 0); - goto out; - } + goto out; /* Also, delete it now that we've opened it, we'll hold * a reference to the fd. If we fail to validate or write, then * the temp space will be cleaned up. */ - (void) unlinkat (_ostree_fetcher_get_dfd (fetcher), temp_path, 0); + ot_cleanup_unlinkat (&tmp_unlinker); if (!validate_bareuseronly_mode (pull_data, checksum, @@ -1046,7 +1044,7 @@ meta_fetch_on_complete (GObject *object, FetchObjectData *fetch_data = user_data; OtPullData *pull_data = fetch_data->pull_data; g_autoptr(GVariant) metadata = NULL; - g_autofree char *temp_path = NULL; + g_auto(OtCleanupUnlinkat) tmp_unlinker = { _ostree_fetcher_get_dfd (fetcher), NULL }; const char *checksum; g_autofree char *checksum_obj = NULL; OstreeObjectType objtype; @@ -1060,7 +1058,7 @@ meta_fetch_on_complete (GObject *object, g_debug ("fetch of %s%s complete", checksum_obj, fetch_data->is_detached_meta ? " (detached)" : ""); - if (!_ostree_fetcher_request_to_tmpfile_finish (fetcher, result, &temp_path, error)) + if (!_ostree_fetcher_request_to_tmpfile_finish (fetcher, result, &tmp_unlinker.path, error)) { if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) { @@ -1103,22 +1101,24 @@ meta_fetch_on_complete (GObject *object, if (objtype == OSTREE_OBJECT_TYPE_TOMBSTONE_COMMIT) goto out; - fd = openat (_ostree_fetcher_get_dfd (fetcher), temp_path, O_RDONLY | O_CLOEXEC); + fd = openat (_ostree_fetcher_get_dfd (fetcher), tmp_unlinker.path, O_RDONLY | O_CLOEXEC); if (fd == -1) { glnx_set_error_from_errno (error); goto out; } + /* Now delete it, keeping the fd open as the last reference); see comment in + * corresponding content fetch path. + */ + ot_cleanup_unlinkat (&tmp_unlinker); + if (fetch_data->is_detached_meta) { if (!ot_util_variant_map_fd (fd, 0, G_VARIANT_TYPE ("a{sv}"), FALSE, &metadata, error)) goto out; - /* Now delete it, see comment in corresponding content fetch path */ - (void) unlinkat (_ostree_fetcher_get_dfd (fetcher), temp_path, 0); - if (!ostree_repo_write_commit_detached_metadata (pull_data->repo, checksum, metadata, pull_data->cancellable, error)) goto out; @@ -1136,8 +1136,6 @@ meta_fetch_on_complete (GObject *object, FALSE, &metadata, error)) goto out; - (void) unlinkat (_ostree_fetcher_get_dfd (fetcher), temp_path, 0); - /* Write the commitpartial file now while we're still fetching data */ if (objtype == OSTREE_OBJECT_TYPE_COMMIT) { diff --git a/src/libotutil/ot-fs-utils.h b/src/libotutil/ot-fs-utils.h index 26c5a499..0f62cb7f 100644 --- a/src/libotutil/ot-fs-utils.h +++ b/src/libotutil/ot-fs-utils.h @@ -49,6 +49,34 @@ ot_link_tmpfile_at (OtTmpfile *tmpf, const char *target, GError **error); + +/* A little helper to call unlinkat() as a cleanup + * function. Mostly only necessary to handle + * deletion of temporary symlinks. + */ +typedef struct { + int dfd; + char *path; +} OtCleanupUnlinkat; + +static inline void +ot_cleanup_unlinkat_clear (OtCleanupUnlinkat *cleanup) +{ + g_clear_pointer (&cleanup->path, g_free); +} + +static inline void +ot_cleanup_unlinkat (OtCleanupUnlinkat *cleanup) +{ + if (cleanup->path) + { + (void) unlinkat (cleanup->dfd, cleanup->path, 0); + ot_cleanup_unlinkat_clear (cleanup); + } +} +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(OtCleanupUnlinkat, ot_cleanup_unlinkat); + + GFile * ot_fdrel_to_gfile (int dfd, const char *path); gboolean ot_readlinkat_gfile_info (int dfd,