From bbb253238a7d42f14fc51779b917e7264a1d936c Mon Sep 17 00:00:00 2001 From: Umang Jain Date: Wed, 18 Jul 2018 11:06:48 +0530 Subject: [PATCH 01/36] Post-release version bump Closes: #1683 Approved by: cgwalters --- configure.ac | 4 ++-- src/libostree/libostree-devel.sym | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 4484e06a..238f51cd 100644 --- a/configure.ac +++ b/configure.ac @@ -4,10 +4,10 @@ dnl update libostree-released.sym from libostree-devel.sym, and update the check dnl in test-symbols.sh, and also set is_release_build=yes below. Then make dnl another post-release commit to bump the version, and set is_release_build=no. m4_define([year_version], [2018]) -m4_define([release_version], [7]) +m4_define([release_version], [8]) m4_define([package_version], [year_version.release_version]) AC_INIT([libostree], [package_version], [walters@verbum.org]) -is_release_build=yes +is_release_build=no AC_CONFIG_HEADER([config.h]) AC_CONFIG_MACRO_DIR([buildutil]) AC_CONFIG_AUX_DIR([build-aux]) diff --git a/src/libostree/libostree-devel.sym b/src/libostree/libostree-devel.sym index a4040ba6..9928ac4f 100644 --- a/src/libostree/libostree-devel.sym +++ b/src/libostree/libostree-devel.sym @@ -18,6 +18,8 @@ ***/ /* Add new symbols here. Release commits should copy this section into -released.sym. */ +LIBOSTREE_2018.8 { +} LIBOSTREE_2018.7; /* Stub section for the stable release *after* this development one; don't * edit this other than to update the last number. This is just a copy/paste From 93da56842259f18bde23daf17dcefcf213b30b81 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 19 Jul 2018 12:46:23 +0000 Subject: [PATCH 02/36] lib/pull: Fix minor memleak in error path Spotted by a downstream Coverity build. Closes: #1684 Approved by: jlebon --- src/libostree/ostree-repo-pull.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 9553272e..a8fee076 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -4131,7 +4131,6 @@ ostree_repo_pull_with_options (OstreeRepo *self, { const char *delta; g_autoptr(GVariant) csum_v = NULL; - guchar *csum_data = g_malloc (OSTREE_SHA256_DIGEST_LEN); g_autoptr(GVariant) ref = g_variant_get_child_value (deltas, i); g_variant_get_child (ref, 0, "&s", &delta); @@ -4140,6 +4139,7 @@ ostree_repo_pull_with_options (OstreeRepo *self, if (!validate_variant_is_csum (csum_v, error)) goto out; + guchar *csum_data = g_malloc (OSTREE_SHA256_DIGEST_LEN); memcpy (csum_data, ostree_checksum_bytes_peek (csum_v), 32); g_hash_table_insert (pull_data->summary_deltas_checksums, g_strdup (delta), From be07c04e638c26df06bfa0c0ee9769faf847bfc4 Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Tue, 24 Jul 2018 14:52:24 -0700 Subject: [PATCH 03/36] lib/repo-commit: Fix min-free-space error message Since min_free_space_size_mb is considered before min_free_space_percent in min_free_space_calculate_reserved_blocks(), it has to be considered first when generating the error message in order for it to be accurate. Closes: #1691 Approved by: jlebon --- src/libostree/ostree-repo-commit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index bbbe1961..39f19362 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -445,10 +445,10 @@ throw_min_free_space_error (OstreeRepo *self, else err_msg = "would be exceeded"; - if (self->min_free_space_percent > 0) - return glnx_throw (error, "min-free-space-percent '%u%%' %s", self->min_free_space_percent, err_msg); - else + if (self->min_free_space_mb > 0) return glnx_throw (error, "min-free-space-size %" G_GUINT64_FORMAT "MB %s", self->min_free_space_mb, err_msg); + else + return glnx_throw (error, "min-free-space-percent '%u%%' %s", self->min_free_space_percent, err_msg); } typedef struct { From 799cfcb5b394812057d23b3af82fb7fc4ff1eeb7 Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Sun, 22 Jul 2018 19:19:52 -0700 Subject: [PATCH 04/36] man/ostree.repo-config: Update min-free-space-* docs Now that it's possible to have both min-free-space-size and min-free-space-percent set in a repo config, update the docs to make the behavior clear in that case. Closes: #1687 Approved by: jlebon --- man/ostree.repo-config.xml | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index 4aedc138..0d39f41e 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -121,18 +121,21 @@ Boston, MA 02111-1307, USA. min-free-space-percent Integer percentage value (0-99) that specifies a minimum percentage of total space (in blocks) in the underlying filesystem to - keep free. The default value is 3. + keep free. The default value is 3. This default value is enforced when none + of min-free-space-* options are set. In case this option is co-existing with min-free-space-size (see below), - it would be ignored and min-free-space-size check would be enforced instead. + and min-free-space-size is set to a non-zero value, min-free-space-percent + would be ignored and min-free-space-size check would be enforced instead. min-free-space-size Value (in MB, GB or TB) that specifies a minimum space in the - underlying filesystem to keep free. Also, note that min-free-space-percent - and min-free-space-size are mutually exclusive. Examples of acceptable values: - 500MB, 1GB etc. The default value is 0MB, which disables this check. + underlying filesystem to keep free. Examples of acceptable values: 500MB, 1GB + etc. + In case this option is co-existing with min-free-space-percent (see above), + and set to a non-zero value, this option takes priority. From 9482922e5e813290ee9952a55f2b574bb61b1ef6 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 25 Jul 2018 17:45:51 -0400 Subject: [PATCH 05/36] lib: Check for NULL pointers in some more places In `write_metadata_object()`, make sure when creating tombstone commits that we're actually passed an expected checksum to use. In `write_dir_entry_to_mtree_internal()`, sanity check that `dfd_iter` is indeed not `NULL` before trying to dereference it. Discovered by Coverity. Closes: #1692 Approved by: cgwalters --- src/libostree/ostree-repo-commit.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 39f19362..632e396c 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -1295,6 +1295,7 @@ write_metadata_object (OstreeRepo *self, char actual_checksum[OSTREE_SHA256_STRING_LEN+1]; if (is_tombstone) { + g_assert (expected_checksum != NULL); memcpy (actual_checksum, expected_checksum, sizeof (actual_checksum)); } else @@ -3309,6 +3310,7 @@ write_dir_entry_to_mtree_internal (OstreeRepo *self, } else { + g_assert (dfd_iter != NULL); g_auto(GLnxDirFdIterator) child_dfd_iter = { 0, }; if (!glnx_dirfd_iterator_init_at (dfd_iter->fd, name, FALSE, &child_dfd_iter, error)) From fcd31a195be0e48709ff44b24ed3b5dc2f4f60e3 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 25 Jul 2018 17:49:12 -0400 Subject: [PATCH 06/36] lib: Fix some minor memory leaks I initially was going to add a `G_DEFINE_AUTOPTR_CLEANUP_FUNC` for `FetchStaticDeltaData`, but it honestly didn't seem worth mucking around ownership everywhere and potentially getting it wrong. Discovered by Coverity. Closes: #1692 Approved by: cgwalters --- src/libostree/ostree-repo-pull.c | 5 ++++- src/libostree/ostree-repo-static-delta-compilation.c | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index a8fee076..da46ed32 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -2486,7 +2486,10 @@ process_one_static_delta (OtPullData *pull_data, OSTREE_STATIC_DELTA_OPEN_FLAGS_SKIP_CHECKSUM, NULL, &inline_delta_part, cancellable, error)) - return FALSE; + { + fetch_static_delta_data_free (fetch_data); + return FALSE; + } _ostree_static_delta_part_execute_async (pull_data->repo, fetch_data->objects, diff --git a/src/libostree/ostree-repo-static-delta-compilation.c b/src/libostree/ostree-repo-static-delta-compilation.c index 9084a72f..f06fad68 100644 --- a/src/libostree/ostree-repo-static-delta-compilation.c +++ b/src/libostree/ostree-repo-static-delta-compilation.c @@ -313,14 +313,13 @@ finish_part (OstreeStaticDeltaBuilder *builder, GError **error) static OstreeStaticDeltaPartBuilder * allocate_part (OstreeStaticDeltaBuilder *builder, GError **error) { - OstreeStaticDeltaPartBuilder *part = g_new0 (OstreeStaticDeltaPartBuilder, 1); - if (builder->parts->len > 0) { if (!finish_part (builder, error)) return NULL; } + OstreeStaticDeltaPartBuilder *part = g_new0 (OstreeStaticDeltaPartBuilder, 1); part->objects = g_ptr_array_new_with_free_func ((GDestroyNotify)g_variant_unref); part->payload = g_string_new (NULL); part->operations = g_string_new (NULL); From 968e8805b0dd4afbcb25db312f53584b0ec59931 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Wed, 25 Jul 2018 17:51:01 -0400 Subject: [PATCH 07/36] lib: Fix some logic/error-checking code Using `MAX(0, $x)` here is useless since we're comparing against an unsigned integer. Just unpack this and only subtract if it's safe to do so. Also, explicitly check for `fd >= 0` rather than just `!= -1` to be sure it's a valid fd. And finally, explicitly check the return value of `g_input_stream_read_all` as is done everywhere else in the tree and make it clear that we're purposely ignoring the return value of `_flush` here, but not in other places. Discovered by Coverity. Closes: #1692 Approved by: cgwalters --- src/libostree/ostree-repo.c | 7 ++++--- src/libotutil/ot-gpg-utils.c | 8 +++----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index f4dcb703..92148b03 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4611,8 +4611,9 @@ ostree_repo_pull_default_console_progress_changed (OstreeAsyncProgress *progress if (bytes_sec > 0) { - /* MAX(0, value) here just to be defensive */ - guint64 est_time_remaining = MAX(0, (total_delta_part_size - fetched_delta_part_size)) / bytes_sec; + guint64 est_time_remaining = 0; + if (total_delta_part_size > fetched_delta_part_size) + est_time_remaining = (total_delta_part_size - fetched_delta_part_size) / bytes_sec; g_autofree char *formatted_est_time_remaining = _formatted_time_remaining_from_seconds (est_time_remaining); /* No space between %s and remaining, since formatted_est_time_remaining has a trailing space */ g_string_append_printf (buf, "Receiving delta parts: %u/%u %s/%s %s/s %sremaining", @@ -4891,7 +4892,7 @@ ostree_repo_add_gpg_signature_summary (OstreeRepo *self, g_autoptr(GVariant) metadata = NULL; if (!ot_openat_ignore_enoent (self->repo_dir_fd, "summary.sig", &fd, error)) return FALSE; - if (fd != -1) + if (fd >= 0) { if (!ot_variant_read_fd (fd, 0, G_VARIANT_TYPE (OSTREE_SUMMARY_SIG_GVARIANT_STRING), FALSE, &metadata, error)) diff --git a/src/libotutil/ot-gpg-utils.c b/src/libotutil/ot-gpg-utils.c index 9d5c8d3a..cc5b0ae4 100644 --- a/src/libotutil/ot-gpg-utils.c +++ b/src/libotutil/ot-gpg-utils.c @@ -262,10 +262,8 @@ data_read_cb (void *handle, void *buffer, size_t size) g_return_val_if_fail (G_IS_INPUT_STREAM (input_stream), -1); - g_input_stream_read_all (input_stream, buffer, size, - &bytes_read, NULL, &local_error); - - if (local_error != NULL) + if (!g_input_stream_read_all (input_stream, buffer, size, + &bytes_read, NULL, &local_error)) { set_errno_from_gio_error (local_error); g_clear_error (&local_error); @@ -287,7 +285,7 @@ data_write_cb (void *handle, const void *buffer, size_t size) if (g_output_stream_write_all (output_stream, buffer, size, &bytes_written, NULL, &local_error)) { - g_output_stream_flush (output_stream, NULL, &local_error); + (void)g_output_stream_flush (output_stream, NULL, &local_error); } if (local_error != NULL) From fb36b62f331a7cc5d3d10e53d6db086bf1c18e0c Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Mon, 16 Jul 2018 14:54:57 -0700 Subject: [PATCH 08/36] lib/repo: Take exclusive lock while generating summary This ensures that commits aren't deleted and refs aren't added, removed, or updated while the summary is being generated. This is in preparation for adding a repo config option that will automatically regenerate the summary on every ref change. Closes: #1681 Approved by: jlebon --- src/libostree/ostree-repo.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 92148b03..7acd0940 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5394,6 +5394,8 @@ summary_add_ref_entry (OstreeRepo *self, * file, listed under the %OSTREE_SUMMARY_COLLECTION_MAP key. Collection IDs * and refs in %OSTREE_SUMMARY_COLLECTION_MAP are guaranteed to be in * lexicographic order. + * + * Locking: exclusive */ gboolean ostree_repo_regenerate_summary (OstreeRepo *self, @@ -5401,6 +5403,18 @@ ostree_repo_regenerate_summary (OstreeRepo *self, GCancellable *cancellable, GError **error) { + /* Take an exclusive lock. This makes sure the commits and deltas don't get + * deleted while generating the summary. It also means we can be sure refs + * won't be created/updated/deleted during the operation, without having to + * add exclusive locks to those operations which would prevent concurrent + * commits from working. + */ + g_autoptr(OstreeRepoAutoLock) lock = NULL; + lock = _ostree_repo_auto_lock_push (self, OSTREE_REPO_LOCK_EXCLUSIVE, + cancellable, error); + if (!lock) + return FALSE; + g_auto(GVariantDict) additional_metadata_builder = OT_VARIANT_BUILDER_INITIALIZER; g_variant_dict_init (&additional_metadata_builder, additional_metadata); g_autoptr(GVariantBuilder) refs_builder = g_variant_builder_new (G_VARIANT_TYPE ("a(s(taya{sv}))")); From 6869bada49ec414837c2b936a761652c66d3f31e Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Fri, 13 Jul 2018 15:53:21 -0700 Subject: [PATCH 09/36] config: Add a core/change-update-summary option This commits adds and implements a boolean repo config option called "change-update-summary" which updates the summary file every time a ref changes (additions, updates, and deletions). The main impetus for this feature is that the `ostree create-usb` and `flatpak create-usb` commands depend on the repo summary being up to date. On the command line you can work around this by asking the user to run `ostree summary --update` but in the case of GNOME Software calling out to `flatpak create-usb` this wouldn't work because it's running as a user and the repo is owned by root. That strategy also means flatpak can't update the repo metadata refs for fear of invalidating the summary. Another use case for this relates to LAN updates. Specifically, the component of eos-updater that generates DNS-SD records advertising ostree refs depends on the repo summary being up to date. Since ostree_repo_regenerate_summary() now takes an exclusive lock, this should be safe to enable. However it's not enabled by default because of the performance cost, and because it's more useful on clients than servers (which likely have another mechanism for updating the summary). Fixes https://github.com/ostreedev/ostree/issues/1664 Closes: #1681 Approved by: jlebon --- man/ostree.repo-config.xml | 10 +++++++ src/libostree/ostree-repo-commit.c | 6 +++++ src/libostree/ostree-repo-private.h | 5 ++++ src/libostree/ostree-repo-refs.c | 5 ++++ src/libostree/ostree-repo.c | 25 +++++++++++++++++- src/ostree/ot-builtin-commit.c | 37 +++++++++++++++++--------- tests/test-auto-summary.sh | 41 +++++++++++++++++++++++++++++ 7 files changed, 115 insertions(+), 14 deletions(-) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index 0d39f41e..6149248f 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -93,6 +93,16 @@ Boston, MA 02111-1307, USA. to false. + + change-update-summary + Boolean value controlling whether or not to + automatically update the summary file after any ref is added, + removed, or updated. This covers a superset of the cases covered by + commit-update-summary, with the exception of orphan commits which + shouldn't affect the summary anyway. Defaults to false. + + + fsync Boolean value controlling whether or not to diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 632e396c..12ee6888 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2202,6 +2202,12 @@ ostree_repo_commit_transaction (OstreeRepo *self, return FALSE; g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy); + /* Update the summary if change-update-summary is set, because doing so was + * delayed for each ref change during the transaction. + */ + if (!_ostree_repo_maybe_regenerate_summary (self, cancellable, error)) + return FALSE; + self->in_transaction = FALSE; if (!ot_ensure_unlinked_at (self->repo_dir_fd, "transaction", 0)) diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 0a447634..4e504326 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -437,6 +437,11 @@ _ostree_repo_get_remote_inherited (OstreeRepo *self, const char *name, GError **error); +gboolean +_ostree_repo_maybe_regenerate_summary (OstreeRepo *self, + GCancellable *cancellable, + GError **error); + /* Locking APIs are currently private. * See https://github.com/ostreedev/ostree/pull/1555 */ diff --git a/src/libostree/ostree-repo-refs.c b/src/libostree/ostree-repo-refs.c index 2600cb7c..83d11c1b 100644 --- a/src/libostree/ostree-repo-refs.c +++ b/src/libostree/ostree-repo-refs.c @@ -1144,6 +1144,11 @@ _ostree_repo_write_ref (OstreeRepo *self, if (!_ostree_repo_update_mtime (self, error)) return FALSE; + /* Update the summary after updating the mtime so the summary doesn't look + * out of date */ + if (!self->in_transaction && !_ostree_repo_maybe_regenerate_summary (self, cancellable, error)) + return FALSE; + return TRUE; } diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 7acd0940..10f87dba 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5386,7 +5386,8 @@ summary_add_ref_entry (OstreeRepo *self, * will aid clients in working out when to check for updates. * * It is regenerated automatically after a commit if - * `core/commit-update-summary` is set. + * `core/commit-update-summary` is set, and automatically after any ref is + * added, removed, or updated if `core/change-update-summary` is set. * * If the `core/collection-id` key is set in the configuration, it will be * included as %OSTREE_SUMMARY_COLLECTION_ID in the summary file. Refs that @@ -5592,6 +5593,28 @@ ostree_repo_regenerate_summary (OstreeRepo *self, return TRUE; } +/* Regenerate the summary if `core/change-update-summary` is set */ +gboolean +_ostree_repo_maybe_regenerate_summary (OstreeRepo *self, + GCancellable *cancellable, + GError **error) +{ + gboolean update_summary; + + if (!ot_keyfile_get_boolean_with_default (self->config, "core", + "change-update-summary", FALSE, + &update_summary, error)) + return FALSE; + + if (update_summary && !ostree_repo_regenerate_summary (self, + NULL, + cancellable, + error)) + return FALSE; + + return TRUE; +} + gboolean _ostree_repo_is_locked_tmpdir (const char *filename) { diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c index c2f78700..ded6522f 100644 --- a/src/ostree/ot-builtin-commit.c +++ b/src/ostree/ot-builtin-commit.c @@ -752,8 +752,8 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio if (!skip_commit) { - gboolean update_summary; guint64 timestamp; + gboolean change_update_summary; if (!opt_no_bindings) { @@ -824,21 +824,32 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio if (!ostree_repo_commit_transaction (repo, &stats, cancellable, error)) goto out; - /* The default for this option is FALSE, even for archive repos, - * because ostree supports multiple processes committing to the same - * repo (but different refs) concurrently, and in fact gnome-continuous - * actually does this. In that context it's best to update the summary - * explicitly instead of automatically here. */ if (!ot_keyfile_get_boolean_with_default (ostree_repo_get_config (repo), "core", - "commit-update-summary", FALSE, - &update_summary, error)) + "change-update-summary", FALSE, + &change_update_summary, error)) goto out; - if (update_summary && !ostree_repo_regenerate_summary (repo, - NULL, - cancellable, - error)) - goto out; + /* No need to update it again if we did for each ref change */ + if (opt_orphan || !change_update_summary) + { + gboolean commit_update_summary; + + /* The default for this option is FALSE, even for archive repos, + * because ostree supports multiple processes committing to the same + * repo (but different refs) concurrently, and in fact gnome-continuous + * actually does this. In that context it's best to update the summary + * explicitly instead of automatically here. */ + if (!ot_keyfile_get_boolean_with_default (ostree_repo_get_config (repo), "core", + "commit-update-summary", FALSE, + &commit_update_summary, error)) + goto out; + + if (commit_update_summary && !ostree_repo_regenerate_summary (repo, + NULL, + cancellable, + error)) + goto out; + } } else { diff --git a/tests/test-auto-summary.sh b/tests/test-auto-summary.sh index b12f1593..5811fcde 100755 --- a/tests/test-auto-summary.sh +++ b/tests/test-auto-summary.sh @@ -29,6 +29,7 @@ echo "1..4" setup_test_repository "bare" echo "ok setup" +# Check that without commit-update-summary set, creating a commit doesn't update the summary mkdir test echo hello > test/a @@ -47,6 +48,7 @@ echo "ok commit 2" assert_streq "$OLD_MD5" "$(md5sum repo/summary)" +# Check that with commit-update-summary set, creating a commit updates the summary $OSTREE --repo=repo config set core.commit-update-summary true echo hello3 > test/a @@ -56,7 +58,46 @@ echo "ok commit 3" assert_not_streq "$OLD_MD5" "$(md5sum repo/summary)" +$OSTREE --repo=repo config set core.commit-update-summary false + # Check that summary --update deletes the .sig file touch repo/summary.sig $OSTREE summary --update assert_not_has_file repo/summary.sig + +# Check that without change-update-summary set, adding, changing, or deleting a ref doesn't update the summary +$OSTREE summary --update +OLD_MD5=$(md5sum repo/summary) +$OSTREE commit -b test2 -s "A commit" test + +assert_streq "$OLD_MD5" "$(md5sum repo/summary)" + +$OSTREE reset test test^ + +assert_streq "$OLD_MD5" "$(md5sum repo/summary)" + +$OSTREE refs --delete test + +assert_streq "$OLD_MD5" "$(md5sum repo/summary)" + +# Check that with change-update-summary set, adding, changing, or deleting a ref updates the summary +$OSTREE --repo=repo config set core.change-update-summary true + +$OSTREE summary --update +OLD_MD5=$(md5sum repo/summary) +echo hello > test/a +$OSTREE commit -b test -s "A commit" test +echo hello2 > test/a +$OSTREE commit -b test -s "Another commit" test + +assert_not_streq "$OLD_MD5" "$(md5sum repo/summary)" + +OLD_MD5=$(md5sum repo/summary) +$OSTREE reset test test^ + +assert_not_streq "$OLD_MD5" "$(md5sum repo/summary)" + +OLD_MD5=$(md5sum repo/summary) +$OSTREE refs --delete test + +assert_not_streq "$OLD_MD5" "$(md5sum repo/summary)" From 61c37aa40cb19b0bb631bf5498ee517b020e1759 Mon Sep 17 00:00:00 2001 From: bubblemelon <12985181+Bubblemelon@users.noreply.github.com> Date: Tue, 24 Jul 2018 11:27:21 -0700 Subject: [PATCH 10/36] bin/refs: Clarify --create error message Fix ref create error when existing rev not specified. Closes: #1690 Approved by: jlebon --- src/ostree/ot-builtin-refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ostree/ot-builtin-refs.c b/src/ostree/ot-builtin-refs.c index a7f77ac8..5c2214cc 100644 --- a/src/ostree/ot-builtin-refs.c +++ b/src/ostree/ot-builtin-refs.c @@ -293,7 +293,7 @@ ostree_builtin_refs (int argc, char **argv, OstreeCommandInvocation *invocation, else if (opt_create) { g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "You must specify an existing ref when creating a new ref"); + "You must specify a revision when creating a new ref"); goto out; } From dcd1522969a36eb4bed7a001154342355d0539ef Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 31 Jul 2018 17:04:26 -0400 Subject: [PATCH 11/36] ostree-remount.service: RemainAfterExit=yes This is standard practice for units like this; e.g. it's what `systemd-remount-fs.service` does. I think it may be part of or the whole cause for https://github.com/projectatomic/rpm-ostree/issues/1471 I haven't reproduced the problem exactly but it seems to me that if the unit starts and is GC'd, then when systemd goes to execute a later unit it might end up restarting it. A noticeable side effect of this is that `systemctl status ostree-remount` exits with code `0` as expected. Closes: #1697 Approved by: jlebon --- src/boot/ostree-remount.service | 1 + 1 file changed, 1 insertion(+) diff --git a/src/boot/ostree-remount.service b/src/boot/ostree-remount.service index 68209f96..47e1387a 100644 --- a/src/boot/ostree-remount.service +++ b/src/boot/ostree-remount.service @@ -31,6 +31,7 @@ Before=systemd-tmpfiles-setup.service [Service] Type=oneshot +RemainAfterExit=yes ExecStart=/usr/lib/ostree/ostree-remount StandardInput=null StandardOutput=syslog From daa57b463062398be70226c5a175618b1b74b1f8 Mon Sep 17 00:00:00 2001 From: Matthew Leeds Date: Mon, 30 Jul 2018 20:05:56 -0700 Subject: [PATCH 12/36] lib/repo-pull: Use correct keyring for dynamic remotes Normally, a configured remote will only serve refs with one associated collection ID, but temporary remotes such as USB drives or LAN peers can serve refs from multiple collection IDs which may use different GPG keyrings. So the OstreeRepoFinderMount and OstreeRepoFinderAvahi classes create dynamic OstreeRemote objects for each (uri, keyring) pair. So if for example the USB mounted at /mnt/usb serves content from the configured remotes "eos-apps" and "eos-sdk", the OstreeRepoFinderResult array returned by ostree_repo_find_remotes_async() will have one result with a remote called something like file_mnt_usb_eos-apps.trustedkeys.gpg and the list of refs on the USB that came from eos-apps, and another result with a remote file_mnt_usb_eos-sdk.trustedkeys.gpg and the list of refs from eos-sdk. Unfortunately while OstreeRepoFinderMount and OstreeRepoFinderAvahi correctly only include refs in a result if the ref uses the associated keyring, the find_remotes_cb() function used to clean up the set of results looks at the remote summary file and includes every ref that's in the intersection with the requested refs, regardless of whether it uses a different remote's keyring. This leads to an error when you try to pull from a USB containing refs from different collection IDs: the pull using the wrong collection ID will error out with "Refspec not found" and the result with the correct keyring will then be ignored "as it has no relevant refs or they have already been pulled." So the pull ultimately fails. This commit fixes the issue by filtering refs coming from a dynamic remote, so that only ones with the collection ID associated with the keyring remote are examined. This only needs to be done for dynamic remotes because you should be able to pull any ref from a configured remote using its keyring. It's also only done when looking at the collection map in the summary file, because LAN/USB remotes won't have a "main" collection ID set (OSTREE_SUMMARY_COLLECTION_ID). Closes: #1695 Approved by: pwithnall --- src/libostree/ostree-repo-pull.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index da46ed32..2a0f1c98 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -5258,8 +5258,9 @@ find_remotes_cb (GObject *obj, summary_bytes, FALSE); /* Check the summary’s additional metadata and set up @commit_metadata - * and @refs_and_remotes_table with all the refs listed in the summary - * file which intersect with @refs. */ + * and @refs_and_remotes_table with the refs listed in the summary file, + * filtered by the keyring associated with this result and the + * intersection with @refs. */ additional_metadata_v = g_variant_get_child_value (summary_v, 1); if (g_variant_lookup (additional_metadata_v, OSTREE_SUMMARY_COLLECTION_ID, "s", &summary_collection_id)) @@ -5280,6 +5281,13 @@ find_remotes_cb (GObject *obj, while (summary_collection_map != NULL && g_variant_iter_loop (summary_collection_map, "{s@a(s(taya{sv}))}", &summary_collection_id, &summary_refs)) { + /* Exclude refs that don't use the associated keyring if this is a + * dynamic remote, by comparing against the collection ID of the + * remote this one inherits from */ + if (result->remote->refspec_name != NULL && + !check_remote_matches_collection_id (self, result->remote->refspec_name, summary_collection_id)) + continue; + if (!find_remotes_process_refs (self, refs, result, i, summary_collection_id, summary_refs, commit_metadatas, refs_and_remotes_table)) { From 3e96ec9811b5cfc5481f8b6b06c8d34d9a35408e Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 30 Jul 2018 10:40:20 -0400 Subject: [PATCH 13/36] lib/refs: Use GLNX_HASH_TABLE_FOREACH_KV helper Closes: #1693 Approved by: mwleeds --- src/libostree/ostree-repo-refs.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/libostree/ostree-repo-refs.c b/src/libostree/ostree-repo-refs.c index 83d11c1b..b38489c4 100644 --- a/src/libostree/ostree-repo-refs.c +++ b/src/libostree/ostree-repo-refs.c @@ -1158,17 +1158,10 @@ _ostree_repo_update_refs (OstreeRepo *self, GCancellable *cancellable, GError **error) { - GHashTableIter hash_iter; - gpointer key, value; - - g_hash_table_iter_init (&hash_iter, refs); - while (g_hash_table_iter_next (&hash_iter, &key, &value)) + GLNX_HASH_TABLE_FOREACH_KV (refs, const char*, refspec, const char*, rev) { - const char *refspec = key; - const char *rev = value; g_autofree char *remote = NULL; g_autofree char *ref_name = NULL; - if (!ostree_parse_refspec (refspec, &remote, &ref_name, error)) return FALSE; From 786ee6bdec8cac24b127af7828659d71702d4f8e Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 30 Jul 2018 10:46:58 -0400 Subject: [PATCH 14/36] lib/config: Rename change-update-summary to auto-... Mildly bikeshed, though I find the name `auto-update-summary` to be easier to grok than `change-update-summary`. I think it's because it can be read as "verb-verb-noun" rather than "noun-verb-noun". Closes: #1693 Approved by: mwleeds --- man/ostree.repo-config.xml | 2 +- src/libostree/ostree-repo-commit.c | 2 +- src/libostree/ostree-repo.c | 16 +++++++--------- src/ostree/ot-builtin-commit.c | 8 ++++---- tests/test-auto-summary.sh | 6 +++--- 5 files changed, 16 insertions(+), 18 deletions(-) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index 6149248f..5424467c 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -94,7 +94,7 @@ Boston, MA 02111-1307, USA. - change-update-summary + auto-update-summary Boolean value controlling whether or not to automatically update the summary file after any ref is added, removed, or updated. This covers a superset of the cases covered by diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 12ee6888..dd225e63 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2202,7 +2202,7 @@ ostree_repo_commit_transaction (OstreeRepo *self, return FALSE; g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy); - /* Update the summary if change-update-summary is set, because doing so was + /* Update the summary if auto-update-summary is set, because doing so was * delayed for each ref change during the transaction. */ if (!_ostree_repo_maybe_regenerate_summary (self, cancellable, error)) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 10f87dba..00a6b460 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5387,7 +5387,7 @@ summary_add_ref_entry (OstreeRepo *self, * * It is regenerated automatically after a commit if * `core/commit-update-summary` is set, and automatically after any ref is - * added, removed, or updated if `core/change-update-summary` is set. + * added, removed, or updated if `core/auto-update-summary` is set. * * If the `core/collection-id` key is set in the configuration, it will be * included as %OSTREE_SUMMARY_COLLECTION_ID in the summary file. Refs that @@ -5593,23 +5593,21 @@ ostree_repo_regenerate_summary (OstreeRepo *self, return TRUE; } -/* Regenerate the summary if `core/change-update-summary` is set */ +/* Regenerate the summary if `core/auto-update-summary` is set */ gboolean _ostree_repo_maybe_regenerate_summary (OstreeRepo *self, GCancellable *cancellable, GError **error) { - gboolean update_summary; + gboolean auto_update_summary; if (!ot_keyfile_get_boolean_with_default (self->config, "core", - "change-update-summary", FALSE, - &update_summary, error)) + "auto-update-summary", FALSE, + &auto_update_summary, error)) return FALSE; - if (update_summary && !ostree_repo_regenerate_summary (self, - NULL, - cancellable, - error)) + if (auto_update_summary && + !ostree_repo_regenerate_summary (self, NULL, cancellable, error)) return FALSE; return TRUE; diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c index ded6522f..6d295d6b 100644 --- a/src/ostree/ot-builtin-commit.c +++ b/src/ostree/ot-builtin-commit.c @@ -753,7 +753,7 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio if (!skip_commit) { guint64 timestamp; - gboolean change_update_summary; + gboolean auto_update_summary; if (!opt_no_bindings) { @@ -825,12 +825,12 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio goto out; if (!ot_keyfile_get_boolean_with_default (ostree_repo_get_config (repo), "core", - "change-update-summary", FALSE, - &change_update_summary, error)) + "auto-update-summary", FALSE, + &auto_update_summary, error)) goto out; /* No need to update it again if we did for each ref change */ - if (opt_orphan || !change_update_summary) + if (opt_orphan || !auto_update_summary) { gboolean commit_update_summary; diff --git a/tests/test-auto-summary.sh b/tests/test-auto-summary.sh index 5811fcde..3a04f184 100755 --- a/tests/test-auto-summary.sh +++ b/tests/test-auto-summary.sh @@ -65,7 +65,7 @@ touch repo/summary.sig $OSTREE summary --update assert_not_has_file repo/summary.sig -# Check that without change-update-summary set, adding, changing, or deleting a ref doesn't update the summary +# Check that without auto-update-summary set, adding, changing, or deleting a ref doesn't update the summary $OSTREE summary --update OLD_MD5=$(md5sum repo/summary) $OSTREE commit -b test2 -s "A commit" test @@ -80,8 +80,8 @@ $OSTREE refs --delete test assert_streq "$OLD_MD5" "$(md5sum repo/summary)" -# Check that with change-update-summary set, adding, changing, or deleting a ref updates the summary -$OSTREE --repo=repo config set core.change-update-summary true +# Check that with auto-update-summary set, adding, changing, or deleting a ref updates the summary +$OSTREE --repo=repo config set core.auto-update-summary true $OSTREE summary --update OLD_MD5=$(md5sum repo/summary) From 72a54fa877e7b459ab4dc19dbb5cc288f219b41c Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 30 Jul 2018 10:54:15 -0400 Subject: [PATCH 15/36] lib/config: Deprecate commit-update-summary option Now that we have `auto-update-summary`, there is no point in having `commit-update-summary`. The latter also only had an effect through the `commit` CLI command, whereas the former is embedded directly in libostree. There is one corner case that slips through: `commit` would update the summary file even if orphan commits were created, which we no longer do here. I can't imagine anyone relying on this, so it seems safe to drop. Closes: #1689 Closes: #1693 Approved by: mwleeds --- man/ostree.repo-config.xml | 20 ++++++++++---------- src/libostree/ostree-repo.c | 20 +++++++++++++++----- src/ostree/ot-builtin-commit.c | 28 ---------------------------- 3 files changed, 25 insertions(+), 43 deletions(-) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index 5424467c..bb406a2e 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -86,23 +86,23 @@ Boston, MA 02111-1307, USA. Currently, this must be set to 1. - - commit-update-summary - Boolean value controlling whether or not to - automatically update the summary file after a commit. Defaults - to false. - - auto-update-summary Boolean value controlling whether or not to automatically update the summary file after any ref is added, - removed, or updated. This covers a superset of the cases covered by - commit-update-summary, with the exception of orphan commits which - shouldn't affect the summary anyway. Defaults to false. + removed, or updated. Other modifications which may render a + summary file stale (like static deltas, or collection IDs) do + not currently trigger an auto-update. + + commit-update-summary + This option is deprecated. Use + auto-update-summary instead, for which this + option is now an alias. + + fsync Boolean value controlling whether or not to diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 00a6b460..922e8a66 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5385,8 +5385,7 @@ summary_add_ref_entry (OstreeRepo *self, * regular, setting the `ostree.summary.expires` key in @additional_metadata * will aid clients in working out when to check for updates. * - * It is regenerated automatically after a commit if - * `core/commit-update-summary` is set, and automatically after any ref is + * It is regenerated automatically after any ref is * added, removed, or updated if `core/auto-update-summary` is set. * * If the `core/collection-id` key is set in the configuration, it will be @@ -5593,20 +5592,31 @@ ostree_repo_regenerate_summary (OstreeRepo *self, return TRUE; } -/* Regenerate the summary if `core/auto-update-summary` is set */ +/* Regenerate the summary if `core/auto-update-summary` is set. We default to FALSE for + * this setting because OSTree supports multiple processes committing to the same repo (but + * different refs) concurrently, and in fact gnome-continuous actually does this. In that + * context it's best to update the summary explicitly once at the end of multiple + * transactions instead of automatically here. `auto-update-summary` only updates + * atomically within a transaction. */ gboolean _ostree_repo_maybe_regenerate_summary (OstreeRepo *self, GCancellable *cancellable, GError **error) { gboolean auto_update_summary; - if (!ot_keyfile_get_boolean_with_default (self->config, "core", "auto-update-summary", FALSE, &auto_update_summary, error)) return FALSE; - if (auto_update_summary && + /* Deprecated alias for `auto-update-summary`. */ + gboolean commit_update_summary; + if (!ot_keyfile_get_boolean_with_default (self->config, "core", + "commit-update-summary", FALSE, + &commit_update_summary, error)) + return FALSE; + + if ((auto_update_summary || commit_update_summary) && !ostree_repo_regenerate_summary (self, NULL, cancellable, error)) return FALSE; diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c index 6d295d6b..535239be 100644 --- a/src/ostree/ot-builtin-commit.c +++ b/src/ostree/ot-builtin-commit.c @@ -753,7 +753,6 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio if (!skip_commit) { guint64 timestamp; - gboolean auto_update_summary; if (!opt_no_bindings) { @@ -823,33 +822,6 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio if (!ostree_repo_commit_transaction (repo, &stats, cancellable, error)) goto out; - - if (!ot_keyfile_get_boolean_with_default (ostree_repo_get_config (repo), "core", - "auto-update-summary", FALSE, - &auto_update_summary, error)) - goto out; - - /* No need to update it again if we did for each ref change */ - if (opt_orphan || !auto_update_summary) - { - gboolean commit_update_summary; - - /* The default for this option is FALSE, even for archive repos, - * because ostree supports multiple processes committing to the same - * repo (but different refs) concurrently, and in fact gnome-continuous - * actually does this. In that context it's best to update the summary - * explicitly instead of automatically here. */ - if (!ot_keyfile_get_boolean_with_default (ostree_repo_get_config (repo), "core", - "commit-update-summary", FALSE, - &commit_update_summary, error)) - goto out; - - if (commit_update_summary && !ostree_repo_regenerate_summary (repo, - NULL, - cancellable, - error)) - goto out; - } } else { From 521e0ec3ac87b539190f3a6cc310237fbaf39517 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 30 Jul 2018 11:11:34 -0400 Subject: [PATCH 16/36] lib/commit: Only auto-update summary if refs were written Closes: #1693 Approved by: mwleeds --- src/libostree/ostree-repo-commit.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index dd225e63..d464cd0a 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2195,19 +2195,21 @@ ostree_repo_commit_transaction (OstreeRepo *self, if (self->txn.refs) if (!_ostree_repo_update_refs (self, self->txn.refs, cancellable, error)) return FALSE; - g_clear_pointer (&self->txn.refs, g_hash_table_destroy); if (self->txn.collection_refs) if (!_ostree_repo_update_collection_refs (self, self->txn.collection_refs, cancellable, error)) return FALSE; - g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy); /* Update the summary if auto-update-summary is set, because doing so was * delayed for each ref change during the transaction. */ - if (!_ostree_repo_maybe_regenerate_summary (self, cancellable, error)) + if ((self->txn.refs || self->txn.collection_refs) && + !_ostree_repo_maybe_regenerate_summary (self, cancellable, error)) return FALSE; + g_clear_pointer (&self->txn.refs, g_hash_table_destroy); + g_clear_pointer (&self->txn.collection_refs, g_hash_table_destroy); + self->in_transaction = FALSE; if (!ot_ensure_unlinked_at (self->repo_dir_fd, "transaction", 0)) From 016cae15732e8a6e0d6211c7f790b24e55f1f7cb Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 13 Aug 2018 17:44:27 +0200 Subject: [PATCH 17/36] Fix leak in ostree_repo_list_collection_refs We need to have the g_auto(GLnxDirFdIterator) inside the loop, or we don't correctly clean up when iterating several times. Closes: #1700 Approved by: cgwalters --- src/libostree/ostree-repo-refs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-refs.c b/src/libostree/ostree-repo-refs.c index b38489c4..1bbe3901 100644 --- a/src/libostree/ostree-repo-refs.c +++ b/src/libostree/ostree-repo-refs.c @@ -1251,7 +1251,6 @@ ostree_repo_list_collection_refs (OstreeRepo *self, (GDestroyNotify) ostree_collection_ref_free, g_free); - g_auto(GLnxDirFdIterator) dfd_iter = { 0, }; g_autoptr(GString) base_path = g_string_new (""); const gchar *main_collection_id = ostree_repo_get_collection_id (self); @@ -1277,6 +1276,8 @@ ostree_repo_list_collection_refs (OstreeRepo *self, { const char *refs_dir = *iter; gboolean refs_dir_exists = FALSE; + g_auto(GLnxDirFdIterator) dfd_iter = { 0, }; + if (!ot_dfd_iter_init_allow_noent (self->repo_dir_fd, refs_dir, &dfd_iter, &refs_dir_exists, error)) return FALSE; From 24883db9089de7549ae81ae51248f2e75095e3c5 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Mon, 13 Aug 2018 18:28:22 +0200 Subject: [PATCH 18/36] ostree_repo_static_delta_generate: Fix leak There is no need to ref the argument of g_variant_builder_add_value Closes: #1701 Approved by: jlebon --- src/libostree/ostree-repo-static-delta-compilation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-static-delta-compilation.c b/src/libostree/ostree-repo-static-delta-compilation.c index f06fad68..054ac06f 100644 --- a/src/libostree/ostree-repo-static-delta-compilation.c +++ b/src/libostree/ostree-repo-static-delta-compilation.c @@ -1521,7 +1521,7 @@ ostree_repo_static_delta_generate (OstreeRepo *self, goto out; } - g_variant_builder_add_value (part_headers, g_variant_ref (part_builder->header)); + g_variant_builder_add_value (part_headers, part_builder->header); total_compressed_size += part_builder->compressed_size; total_uncompressed_size += part_builder->uncompressed_size; From 0a53af801e82cadddddd13ce54a304634df08e46 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 14 Aug 2018 10:09:32 +0200 Subject: [PATCH 19/36] ostree_repo_pull_from_remotes_async: Fix leak of options copy_option() unnecessarily passed ownership of the value to g_variant_dict_insert_value, but that already refs, so it was leaked. Closes: #1702 Approved by: cgwalters --- src/libostree/ostree-repo-pull.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 2a0f1c98..80c31ed8 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -5667,7 +5667,7 @@ copy_option (GVariantDict *master_options, { g_autoptr(GVariant) option_v = g_variant_dict_lookup_value (master_options, key, expected_type); if (option_v != NULL) - g_variant_dict_insert_value (slave_options, key, g_steal_pointer (&option_v)); + g_variant_dict_insert_value (slave_options, key, option_v); } /** From 4a389b308294c0dd6a82578ad320703b268f16ba Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Tue, 14 Aug 2018 12:38:15 +0200 Subject: [PATCH 20/36] Avoid race condition in case tests directory does not exist Make sure the tests directory exists before symlinking files into it. Closes: #1703 Closes: #1704 Approved by: cgwalters --- Makefile-tests.am | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile-tests.am b/Makefile-tests.am index 85995aa6..3323c5f3 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -360,7 +360,8 @@ EXTRA_DIST += \ $(NULL) tests/libreaddir-rand.so: Makefile - $(AM_V_GEN) ln -fns ../.libs/libreaddir-rand.so tests + mkdir -p tests/ + $(AM_V_GEN) ln -fns ../.libs/libreaddir-rand.so tests/ ALL_LOCAL_RULES += tests/libreaddir-rand.so CLEANFILES += tests/libreaddir-rand.so tests/ostree-symlink-stamp \ tests/ostree-prepare-root-symlink-stamp tests/ostree-remount-symlink-stamp \ From ce4eb12ffb69196242207e76bcc4ee5dcea0c6c2 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 2 Aug 2018 13:58:56 -0500 Subject: [PATCH 21/36] tests: Add tests for remote summary update races There have been subtle bugs in the past when a client pulls while the remote server is updating the summary. The client may get the old summary and new signature or vice versa. Add tests to simulate this behavior to make sure there aren't regressions in the future. Closes: #1698 Approved by: cgwalters --- tests/test-pull-summary-sigs.sh | 81 ++++++++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) diff --git a/tests/test-pull-summary-sigs.sh b/tests/test-pull-summary-sigs.sh index c6d04f41..77ff8444 100755 --- a/tests/test-pull-summary-sigs.sh +++ b/tests/test-pull-summary-sigs.sh @@ -23,7 +23,7 @@ set -euo pipefail . $(dirname $0)/libtest.sh -echo "1..7" +echo "1..9" COMMIT_SIGN="--gpg-homedir=${TEST_GPG_KEYHOME} --gpg-sign=${TEST_GPG_KEYID_1}" setup_fake_remote_repo1 "archive" "${COMMIT_SIGN}" @@ -151,4 +151,83 @@ grep static-deltas summary.txt > static-deltas.txt assert_file_has_content static-deltas.txt \ $(${OSTREE} --repo=repo rev-parse origin:main) +## Tests for handling of cached summaries while racing with remote summary updates + +# Make 2 different but valid summary/signature pairs to test races with +${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{,.1} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{,.1} +mkdir ${test_tmpdir}/ostree-srv/even-another-files +cd ${test_tmpdir}/ostree-srv/even-another-files +echo 'hello world even another object' > even-another-hello-world +${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo commit ${COMMIT_SIGN} -b even-another -s "A commit" -m "Another Commit body" +${OSTREE} --repo=${test_tmpdir}/ostree-srv/gnomerepo summary -u ${COMMIT_SIGN} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{,.2} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{,.2} +cd ${test_tmpdir} + +# Reset to the old valid summary and pull to cache it +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,} +repo_reinit +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 + +# Simulate a pull race where the client gets the old summary and the new +# summary signature since it was generated on the server between the +# requests +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.2,} +if ${OSTREE} --repo=repo pull origin main 2>err.txt; then + assert_not_reached "Successful pull with old summary" +fi +assert_file_has_content err.txt "none are in trusted keyring" +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 + +# Publish correct summary and check that subsequent pull succeeds +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.2,} +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.2 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.2 >&2 + +echo "ok pull with signed summary remote old summary" + +# Reset to the old valid summary and pull to cache it +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,} +repo_reinit +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 + +# Simulate a pull race where the client gets the new summary and the old +# summary signature. This is unlikely to happen except if the web server +# is caching the old signature. This should succeed because the cached +# old summary is used. +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.2,} +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 + +# Publish correct signature and check that subsequent pull succeeds +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.2,} +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.2 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.2 >&2 + +echo "ok pull with signed summary remote old summary signature" + libtest_cleanup_gpg From 1c69f1ed318ed8b9447119b0311074b89637ad61 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 2 Aug 2018 14:14:10 -0500 Subject: [PATCH 22/36] lib/pull: Add debug message when loading summary from cache This helps when debugging issues with the cached summary handling. Closes: #1698 Approved by: cgwalters --- src/libostree/ostree-repo-pull.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 80c31ed8..9eac6376 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -3986,7 +3986,10 @@ ostree_repo_pull_with_options (OstreeRepo *self, goto out; if (bytes_summary) - summary_from_cache = TRUE; + { + g_debug ("Loaded %s summary from cache", remote_name_or_baseurl); + summary_from_cache = TRUE; + } if (!pull_data->summary && !bytes_summary) { From e5061f54d68ea95ab0b953e6cec6eeca26b01a12 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 2 Aug 2018 15:15:28 -0500 Subject: [PATCH 23/36] lib/pull: Fetch summary if cached version doesn't match signature If for some reason the cached summary doesn't match the cached signature then fetch the remote summary and verify again. Since commit c4c2b5eb this is unlikely to happen since the summary will only be cached if it matches the signature. However, if the summary cache has been corrupted for any other reason then it's best to be safe and fetch the remote summary again. This is essentially the corollary to c4c2b5eb. Where that commit helps you from getting into the corrupted summary cache in the first place, this helps you get out of it. Without this the client can get wedged until a prune or the remote server republishes the summary. Closes: #1698 Approved by: cgwalters --- src/libostree/ostree-repo-pull.c | 37 +++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 9eac6376..f692874d 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -4027,12 +4027,43 @@ ostree_repo_pull_with_options (OstreeRepo *self, if (pull_data->gpg_verify_summary && bytes_summary && bytes_sig) { g_autoptr(OstreeGpgVerifyResult) result = NULL; + g_autoptr(GError) temp_error = NULL; result = ostree_repo_verify_summary (self, pull_data->remote_name, bytes_summary, bytes_sig, - cancellable, error); - if (!ostree_gpg_verify_result_require_valid_signature (result, error)) - goto out; + cancellable, &temp_error); + if (!ostree_gpg_verify_result_require_valid_signature (result, &temp_error)) + { + if (summary_from_cache) + { + /* The cached summary doesn't match, fetch a new one and verify again */ + summary_from_cache = FALSE; + g_clear_pointer (&bytes_summary, (GDestroyNotify)g_bytes_unref); + g_debug ("Remote %s cached summary invalid, pulling new version", + pull_data->remote_name); + if (!_ostree_fetcher_mirrored_request_to_membuf (pull_data->fetcher, + pull_data->meta_mirrorlist, + "summary", + OSTREE_FETCHER_REQUEST_OPTIONAL_CONTENT, + pull_data->n_network_retries, + &bytes_summary, + OSTREE_MAX_METADATA_SIZE, + cancellable, error)) + goto out; + + g_autoptr(OstreeGpgVerifyResult) retry = + ostree_repo_verify_summary (self, pull_data->remote_name, + bytes_summary, bytes_sig, + cancellable, error); + if (!ostree_gpg_verify_result_require_valid_signature (retry, error)) + goto out; + } + else + { + g_propagate_error (error, g_steal_pointer (&temp_error)); + goto out; + } + } } if (bytes_summary) From 1b5cb52da29bccbf14f49b0d990db2b5ca6ada82 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 2 Aug 2018 14:12:32 -0500 Subject: [PATCH 24/36] tests: Test for recovery from corrupted summary cache Check that recovery from a corrupted summary cache (cached summary doesn't match cached signature) works. Closes: #1698 Approved by: cgwalters --- tests/test-pull-summary-sigs.sh | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/test-pull-summary-sigs.sh b/tests/test-pull-summary-sigs.sh index 77ff8444..4b71b506 100755 --- a/tests/test-pull-summary-sigs.sh +++ b/tests/test-pull-summary-sigs.sh @@ -23,7 +23,7 @@ set -euo pipefail . $(dirname $0)/libtest.sh -echo "1..9" +echo "1..10" COMMIT_SIGN="--gpg-homedir=${TEST_GPG_KEYHOME} --gpg-sign=${TEST_GPG_KEYID_1}" setup_fake_remote_repo1 "archive" "${COMMIT_SIGN}" @@ -230,4 +230,37 @@ cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summ echo "ok pull with signed summary remote old summary signature" +# Reset to the old valid summary and pull to cache it +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.1,} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.1,} +repo_reinit +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 + +# Simulate a broken summary cache to see if it can be recovered from. +# Prior to commit c4c2b5eb the client would save the summary to the +# cache before validating the signature. That would mean the cache would +# have mismatched summary and signature and ostree would remain +# deadlocked there until the remote published a new signature. +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.2 repo/tmp/cache/summaries/origin +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.1 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 + +# Publish new signature and check that subsequent pull succeeds +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary{.2,} +cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig{.2,} +${OSTREE} --repo=repo pull origin main +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.2 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.2 >&2 + +echo "ok pull with signed summary broken cache" + libtest_cleanup_gpg From 4e6b13e8b6f63e95793a53930794f997bf46b929 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 10 Aug 2018 11:53:50 -0500 Subject: [PATCH 25/36] repo: Add OSTREE_REPO_TEST_ERROR=invalid-cache env var Add an invalid-cache test error flag to ensure that the code that checks for and recovers from a corrupted summary cache is hit. This helps make sure that the recovery path is actually used without resorting to G_MESSAGES_DEBUG. Closes: #1698 Approved by: cgwalters --- src/libostree/ostree-repo-private.h | 3 ++- src/libostree/ostree-repo-pull.c | 14 ++++++++++++-- src/libostree/ostree-repo.c | 1 + tests/test-pull-summary-sigs.sh | 12 ++++++++++++ 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 4e504326..99eaf494 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -61,7 +61,8 @@ G_BEGIN_DECLS #define OSTREE_COMMIT_TIMESTAMP "ostree.commit.timestamp" typedef enum { - OSTREE_REPO_TEST_ERROR_PRE_COMMIT = (1 << 0) + OSTREE_REPO_TEST_ERROR_PRE_COMMIT = (1 << 0), + OSTREE_REPO_TEST_ERROR_INVALID_CACHE = (1 << 1), } OstreeRepoTestErrorFlags; struct OstreeRepoCommitModifier { diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index f692874d..ed61c97e 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -4037,10 +4037,20 @@ ostree_repo_pull_with_options (OstreeRepo *self, if (summary_from_cache) { /* The cached summary doesn't match, fetch a new one and verify again */ + if ((self->test_error_flags & OSTREE_REPO_TEST_ERROR_INVALID_CACHE) > 0) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Remote %s cached summary invalid and " + "OSTREE_REPO_TEST_ERROR_INVALID_CACHE specified", + pull_data->remote_name); + goto out; + } + else + g_debug ("Remote %s cached summary invalid, pulling new version", + pull_data->remote_name); + summary_from_cache = FALSE; g_clear_pointer (&bytes_summary, (GDestroyNotify)g_bytes_unref); - g_debug ("Remote %s cached summary invalid, pulling new version", - pull_data->remote_name); if (!_ostree_fetcher_mirrored_request_to_membuf (pull_data->fetcher, pull_data->meta_mirrorlist, "summary", diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 922e8a66..4bea0f10 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1187,6 +1187,7 @@ ostree_repo_init (OstreeRepo *self) static gsize gpgme_initialized; const GDebugKey test_error_keys[] = { { "pre-commit", OSTREE_REPO_TEST_ERROR_PRE_COMMIT }, + { "invalid-cache", OSTREE_REPO_TEST_ERROR_INVALID_CACHE }, }; if (g_once_init_enter (&gpgme_initialized)) diff --git a/tests/test-pull-summary-sigs.sh b/tests/test-pull-summary-sigs.sh index 4b71b506..dee186b5 100755 --- a/tests/test-pull-summary-sigs.sh +++ b/tests/test-pull-summary-sigs.sh @@ -245,7 +245,19 @@ cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summ # cache before validating the signature. That would mean the cache would # have mismatched summary and signature and ostree would remain # deadlocked there until the remote published a new signature. +# +# First pull with OSTREE_REPO_TEST_ERROR=invalid-cache to see the +# invalid cache is detected. Then pull again to check if it can be +# recovered from. cp ${test_tmpdir}/ostree-srv/gnomerepo/summary.2 repo/tmp/cache/summaries/origin +if OSTREE_REPO_TEST_ERROR=invalid-cache ${OSTREE} --repo=repo pull origin main 2>err.txt; then + assert_not_reached "Should have hit OSTREE_REPO_TEST_ERROR_INVALID_CACHE" +fi +assert_file_has_content err.txt "OSTREE_REPO_TEST_ERROR_INVALID_CACHE" +assert_has_file repo/tmp/cache/summaries/origin +assert_has_file repo/tmp/cache/summaries/origin.sig +cmp repo/tmp/cache/summaries/origin ${test_tmpdir}/ostree-srv/gnomerepo/summary.2 >&2 +cmp repo/tmp/cache/summaries/origin.sig ${test_tmpdir}/ostree-srv/gnomerepo/summary.sig.1 >&2 ${OSTREE} --repo=repo pull origin main assert_has_file repo/tmp/cache/summaries/origin assert_has_file repo/tmp/cache/summaries/origin.sig From 57dbd176e99b657dba2b7450a5823d0b27c4ff38 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Fri, 17 Aug 2018 21:50:14 -0400 Subject: [PATCH 26/36] ci: Bump rpm-ostree tag we build for tests The latest rpm-ostree release no longer requires `python3-devel` so `dnf builddep` here is no longer pulling it in, subsequently causing issues when building an older rpm-ostree release. Let's just bump the release tag so we don't have to also start keeping track of older dependencies. Closes: #1708 Approved by: cgwalters --- ci/rpmostree.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/rpmostree.sh b/ci/rpmostree.sh index f766a368..0d8a5e41 100755 --- a/ci/rpmostree.sh +++ b/ci/rpmostree.sh @@ -6,7 +6,7 @@ set -xeuo pipefail # Frozen to a tag for now to help predictability; it's # also useful to test building *older* versions since # that must work. -RPMOSTREE_TAG=v2018.5 +RPMOSTREE_TAG=v2018.7 dn=$(dirname $0) . ${dn}/libpaprci/libbuild.sh @@ -48,4 +48,4 @@ if ! make vmsync; then fatal "vmsync failed" fi # Now run tests; just a subset ⊂ for now to avoid CI overload -make vmcheck TESTS="basic layering-basic" +make vmcheck TESTS="layering-basic-1 layering-basic-2" From ada5716fb2843d4cd431150da6a0de0a65d8cb3a Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Fri, 17 Aug 2018 13:14:17 +0100 Subject: [PATCH 27/36] man/ostree.repo-config: improve min-free-space-* docs 0 is not a valid value (the units are required), and 1MB is taken to mean 1048576 bytes, not 1000000 bytes (unlike other human-readable sizes in libostree, which are formatted by g_format_size(), and hence use power-of-10 units). It's probably a bit late to change this latter point, but the documentation should mention it. Define 'MB' etc. more precisely; include the byte counts in the examples; and improve the formatting of the min-free-space-* paragraphs. Signed-off-by: Will Thompson Closes: #1706 Approved by: mwleeds --- man/ostree.repo-config.xml | 40 +++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/man/ostree.repo-config.xml b/man/ostree.repo-config.xml index bb406a2e..dc126d65 100644 --- a/man/ostree.repo-config.xml +++ b/man/ostree.repo-config.xml @@ -129,24 +129,36 @@ Boston, MA 02111-1307, USA. min-free-space-percent - Integer percentage value (0-99) that specifies a minimum - percentage of total space (in blocks) in the underlying filesystem to - keep free. The default value is 3. This default value is enforced when none - of min-free-space-* options are set. - In case this option is co-existing with min-free-space-size (see below), - and min-free-space-size is set to a non-zero value, min-free-space-percent - would be ignored and min-free-space-size check would be enforced instead. - + + + Integer percentage value (0-99) that specifies a minimum percentage + of total space (in blocks) in the underlying filesystem to keep + free. The default value is 3, which is enforced when neither this + option nor min-free-space-size are set. + + + If min-free-space-size is set to a non-zero + value, min-free-space-percent is ignored. + + min-free-space-size - Value (in MB, GB or TB) that specifies a minimum space in the - underlying filesystem to keep free. Examples of acceptable values: 500MB, 1GB - etc. - In case this option is co-existing with min-free-space-percent (see above), - and set to a non-zero value, this option takes priority. - + + + Value (in power-of-2 MB, GB or TB) that specifies a minimum space + in the underlying filesystem to keep free. Examples of acceptable + values: 500MB (524 288 000 bytes), + 1GB (1 073 741 824 bytes), + 1TB (1 099 511 627 776 bytes). + + + If this option is set to a non-zero value, and + min-free-space-percent is also set, this option + takes priority. + + From 2b19869307252b3fc038e4166f6f34a0d1f1e5ac Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Fri, 17 Aug 2018 14:40:01 +0100 Subject: [PATCH 28/36] repo: remove outdated note from write_config() docs Since 9dc6ddce0848cf04b76fc1c673c8fc1a0f6f425a it has not been true that 'new_config' was simply ref'd: it's serialized, and then re-parsed into a new GKeyFile. Closes: #1707 Approved by: jlebon --- src/libostree/ostree-repo.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 4bea0f10..18736c24 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1441,12 +1441,10 @@ ostree_repo_copy_config (OstreeRepo *self) /** * ostree_repo_write_config: * @self: Repo - * @new_config: Overwrite the config file with this data. Do not change later! + * @new_config: Overwrite the config file with this data * @error: a #GError * - * Save @new_config in place of this repository's config file. Note - * that @new_config should not be modified after - this function - * simply adds a reference. + * Save @new_config in place of this repository's config file. */ gboolean ostree_repo_write_config (OstreeRepo *self, From e7305bbc8a262d595c7d1c507be7fba0e5a88d06 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Mon, 21 May 2018 15:11:36 +0100 Subject: [PATCH 29/36] lib/repo-pull: Prefer object pull over from-scratch delta if ref exists If a ref already exists, we are likely only a few commits behind the current head of the ref, so it is probably better for bandwidth consumption to pull the individual objects rather than the from-scratch delta. Signed-off-by: Philip Withnall Closes: #1709 Approved by: cgwalters --- src/libostree/ostree-repo-pull.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index ed61c97e..c6b70ffb 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -3348,6 +3348,19 @@ initiate_request (OtPullData *pull_data, return TRUE; } + /* If doing a delta from a ref, look up the from-revision, since we need it + * on most paths below. */ + if (ref != NULL) + { + g_autofree char *refspec = NULL; + if (pull_data->remote_name != NULL) + refspec = g_strdup_printf ("%s:%s", pull_data->remote_name, ref->ref_name); + if (!ostree_repo_resolve_rev (pull_data->repo, + refspec ?: ref->ref_name, TRUE, + &delta_from_revision, error)) + return FALSE; + } + /* If we have a summary, we can use the newer logic */ if (pull_data->summary) { @@ -3375,7 +3388,16 @@ initiate_request (OtPullData *pull_data, enqueue_one_static_delta_superblock_request (pull_data, deltares.from_revision, to_revision, ref); break; case DELTA_SEARCH_RESULT_SCRATCH: - enqueue_one_static_delta_superblock_request (pull_data, NULL, to_revision, ref); + { + /* If a from-scratch delta is available, we don’t want to use it if + * the ref already exists locally, since we are likely only a few + * commits out of date; so doing an object pull is likely more + * bandwidth efficient. */ + if (delta_from_revision != NULL) + queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); + else + enqueue_one_static_delta_superblock_request (pull_data, NULL, to_revision, ref); + } break; case DELTA_SEARCH_RESULT_UNCHANGED: { @@ -3395,13 +3417,6 @@ initiate_request (OtPullData *pull_data, { /* Are we doing a delta via a ref? In that case we can fall back to the older * logic of just using the current tip of the ref as a delta FROM source. */ - g_autofree char *refspec = NULL; - if (pull_data->remote_name != NULL) - refspec = g_strdup_printf ("%s:%s", pull_data->remote_name, ref->ref_name); - if (!ostree_repo_resolve_rev (pull_data->repo, - refspec ?: ref->ref_name, TRUE, - &delta_from_revision, error)) - return FALSE; /* Determine whether the from revision we have is partial; this * can happen if e.g. one uses `ostree pull --commit-metadata-only`. From dde3f1c0fb4d33c50968159a5843821c976f0c49 Mon Sep 17 00:00:00 2001 From: Sinny Kumari Date: Fri, 17 Aug 2018 21:25:56 +0530 Subject: [PATCH 30/36] src/ostree: Add --group option to ostree config Fetching value from a repo config using 'ostree config get SECTIONNAME.KEYNAME' didn't work in some cases like when having dots in Group Name entry. As per Desktop entry file specification, Group Name may contain all ASCII characters except for [ and ] and control characters. Link - https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-1.1.html Having --group option will help user to clearly specify Group Name and get desired result. It also adds test for ostree config get|set and bash completion for --group option Fixes https://github.com/ostreedev/ostree/issues/1565 Closes: #1696 Approved by: cgwalters --- Makefile-tests.am | 1 + bash/ostree | 1 + man/ostree-config.xml | 6 +-- src/ostree/ot-builtin-config.c | 71 ++++++++++++++++++++++++---------- tests/test-config.sh | 55 ++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 24 deletions(-) create mode 100755 tests/test-config.sh diff --git a/Makefile-tests.am b/Makefile-tests.am index 3323c5f3..9837e5cd 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -134,6 +134,7 @@ _installed_or_uninstalled_test_scripts = \ tests/test-repo-finder-mount-integration.sh \ tests/test-summary-collections.sh \ tests/test-pull-collections.sh \ + tests/test-config.sh \ $(NULL) experimental_test_scripts = \ diff --git a/bash/ostree b/bash/ostree index d7b54373..677aee7c 100644 --- a/bash/ostree +++ b/bash/ostree @@ -398,6 +398,7 @@ _ostree_config() { " local options_with_args=" + --group --repo " diff --git a/man/ostree-config.xml b/man/ostree-config.xml index f1232602..f052aebb 100644 --- a/man/ostree-config.xml +++ b/man/ostree-config.xml @@ -51,10 +51,10 @@ Boston, MA 02111-1307, USA. - ostree config get SECTIONNAME.KEYNAME + ostree config get --group=GROUPNAME KEYNAME - ostree config set SECTIONNAME.KEYNAME VALUE + ostree config set --group=GROUPNAME KEYNAME VALUE @@ -68,7 +68,7 @@ Boston, MA 02111-1307, USA. Example - $ ostree config get core.mode + $ ostree config get --group=core mode bare diff --git a/src/ostree/ot-builtin-config.c b/src/ostree/ot-builtin-config.c index 89bf3df9..b9fa824d 100644 --- a/src/ostree/ot-builtin-config.c +++ b/src/ostree/ot-builtin-config.c @@ -28,12 +28,15 @@ #include "ostree.h" #include "otutil.h" +static char* opt_group; + /* ATTENTION: * Please remember to update the bash-completion script (bash/ostree) and * man page (man/ostree-config.xml) when changing the option list. */ static GOptionEntry options[] = { + { "group", 0, 0, G_OPTION_ARG_STRING, &opt_group , "Group name followed by key for a remote config", NULL }, { NULL } }; @@ -44,7 +47,7 @@ split_key_string (const char *k, GError **error) { const char *dot = strchr (k, '.'); - + if (!dot) { return glnx_throw (error, @@ -85,18 +88,32 @@ ostree_builtin_config (int argc, char **argv, OstreeCommandInvocation *invocatio if (!strcmp (op, "set")) { - if (argc < 4) + printf("GROUP NUMBER = %s %d\n", opt_group, argc); + if (opt_group) { - g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "KEY and VALUE must be specified"); - goto out; + if (argc < 4) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "GROUP name, KEY and VALUE must be specified"); + goto out; + } + section = g_strdup(opt_group); + key = g_strdup(argv[2]); + value = argv[3]; + } + else + { + if (argc < 4) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "KEY and VALUE must be specified"); + goto out; + } + section_key = argv[2]; + value = argv[3]; + if(!split_key_string (section_key, §ion, &key, error)) + goto out; } - - section_key = argv[2]; - value = argv[3]; - - if (!split_key_string (section_key, §ion, &key, error)) - goto out; config = ostree_repo_copy_config (repo); g_key_file_set_string (config, section, key, value); @@ -108,17 +125,29 @@ ostree_builtin_config (int argc, char **argv, OstreeCommandInvocation *invocatio { GKeyFile *readonly_config = NULL; g_autofree char *value = NULL; - if (argc < 3) + if (opt_group) { - g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, - "KEY must be specified"); - goto out; + if (argc < 3) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Group name and key must be specified"); + goto out; + } + section = g_strdup(opt_group); + key = g_strdup(argv[2]); + } + else + { + if(argc < 3) + { + g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, + "KEY must be specified"); + goto out; + } + section_key = argv[2]; + if (!split_key_string (section_key, §ion, &key, error)) + goto out; } - - section_key = argv[2]; - - if (!split_key_string (section_key, §ion, &key, error)) - goto out; readonly_config = ostree_repo_get_config (repo); value = g_key_file_get_string (readonly_config, section, key, error); @@ -133,7 +162,7 @@ ostree_builtin_config (int argc, char **argv, OstreeCommandInvocation *invocatio "Unknown operation %s", op); goto out; } - + ret = TRUE; out: if (config) diff --git a/tests/test-config.sh b/tests/test-config.sh new file mode 100755 index 00000000..b1ea3e5e --- /dev/null +++ b/tests/test-config.sh @@ -0,0 +1,55 @@ +#!/bin/bash +# +# Copyright (C) 2018 Sinny Kumari +# +# SPDX-License-Identifier: LGPL-2.0+ +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the +# Free Software Foundation, Inc., 59 Temple Place - Suite 330, +# Boston, MA 02111-1307, USA. + +set -euo pipefail + +. $(dirname $0)/libtest.sh + +echo '1..2' + +ostree_repo_init repo +${CMD_PREFIX} ostree remote add --repo=repo --set=xa.title=Flathub --set=xa.title-is-set=true flathub https://dl.flathub.org/repo/ +${CMD_PREFIX} ostree remote add --repo=repo org.mozilla.FirefoxRepo http://example.com/ostree/repo/ + +${CMD_PREFIX} ostree config --repo=repo get core.mode > list.txt +${CMD_PREFIX} ostree config --repo=repo get --group=core repo_version >> list.txt +${CMD_PREFIX} ostree config --repo=repo get --group='remote "flathub"' 'xa.title' >> list.txt +${CMD_PREFIX} ostree config --repo=repo get --group='remote "flathub"' 'xa.title-is-set' >> list.txt +${CMD_PREFIX} ostree config --repo=repo get --group='remote "org.mozilla.FirefoxRepo"' url >> list.txt +${CMD_PREFIX} cat list.txt + +assert_file_has_content list.txt "bare" +assert_file_has_content list.txt "1" +assert_file_has_content list.txt "Flathub" +assert_file_has_content list.txt "true" +assert_file_has_content list.txt "http://example.com/ostree/repo/" +echo "ok config get" + +${CMD_PREFIX} ostree config --repo=repo set core.mode bare-user-only +${CMD_PREFIX} ostree config --repo=repo set --group='remote "flathub"' 'xa.title' 'Nightly Flathub' +${CMD_PREFIX} ostree config --repo=repo set --group='remote "flathub"' 'xa.title-is-set' 'false' +${CMD_PREFIX} ostree config --repo=repo set --group='remote "org.mozilla.FirefoxRepo"' url http://example.com/ostree/ + +assert_file_has_content repo/config "bare-user-only" +assert_file_has_content repo/config "Nightly Flathub" +assert_file_has_content repo/config "false" +assert_file_has_content repo/config "http://example.com/ostree/" +echo "ok config set" From 3f1faa4f3396e8ed7c86d21f9b84b9f0c6a08f76 Mon Sep 17 00:00:00 2001 From: Robert Fairley Date: Thu, 16 Aug 2018 16:46:53 -0400 Subject: [PATCH 31/36] docs: Add contributing tutorial This adds a tutorial for developing/contributing to OSTree. The following information is included: - Installing build dependencies for OSTree - Building OSTree, and running in a container and VM - Adding a basic command to OSTree, and testing the command - Suggested git workflows for working with the upstream repo This is helpful to give new contributors a more detailed introduction to developing OSTree, and an idea of what the workflow looks like. Closes: #1694 Approved by: cgwalters --- docs/contributing-tutorial.md | 450 ++++++++++++++++++++++++++++++++++ 1 file changed, 450 insertions(+) create mode 100644 docs/contributing-tutorial.md diff --git a/docs/contributing-tutorial.md b/docs/contributing-tutorial.md new file mode 100644 index 00000000..a1f0c4be --- /dev/null +++ b/docs/contributing-tutorial.md @@ -0,0 +1,450 @@ +# OSTree Contributing Tutorial + +The following guide is about OSTree forking, building, adding a command, testing the command, and submitting the change. + +- [Getting Started](#getting-started) +- [Building OSTree](#building-ostree) + - [Install Build Dependencies](#install-build-dependencies) + - [OSTree Build Commands](#ostree-build-commands) +- [Testing a Build](#testing-a-build) + - [Testing in a Container](#testing-in-a-container) + - [Testing in a Virtual Machine](#testing-in-a-virtual-machine) +- [Tutorial: Adding a basic builtin command to OSTree](#tutorial-adding-a-basic-builtin-command-to-ostree) + - [Modifying OSTree](#modifying-ostree) + - [OSTree Tests](#ostree-tests) + - [Submitting a Patch](#submitting-a-patch) + - [Returning Workflow](#returning-workflow) + +--- + +## Getting Started + +Fork https://github.com/ostreedev/ostree, then run the following commands. + +```bash +$ git clone https://github.com//ostree && cd ostree +$ git remote add upstream https://github.com/ostreedev/ostree +$ git checkout master +$ git fetch upstream && git branch --set-upstream-to=upstream/master master +``` +Make a branch from master for your patch. + +```bash +$ git checkout -b +$ git branch --set-upstream-to=upstream/master +``` + +## Building OSTree + +### Install Build Dependencies + +Execute one of the following group commands as superuser depending on your machine's package manager. + +For Fedora: + +```bash +$ dnf install @buildsys-build dnf-plugins-core && \ +dnf builddep ostree +``` + +For CentOS: + +```bash +$ yum install yum-utils dnf-plugins-core && \ +yum-builddep ostree +``` + +For Debian based distros: + +```bash +$ apt-get update && \ +apt-get install build-essential && \ +apt-get build-dep ostree +``` + +[build.sh](../ci/build.sh) will have a list of packages needed to build ostree. + +### OSTree Build Commands + +These are the basic commands to build OSTree. Depending on the OS that OSTree will be build for, the flags or options for `./autogen.sh` and `./configure` will vary. + +See `ostree-build.sh` in this tutorial below for specific commands to building OSTree for Fedora 28 and Fedora 28 Atomic Host. + +```bash +# optional: autogen.sh will run this if necessary +git submodule update --init + +env NOCONFIGURE=1 ./autogen.sh + +# run ./configure if makefile does not exist +./configure + +make +make install DESTDIR=/path/to/install/binary +``` + +#### Notes + +Running `git submodule update --init` is optional since `autogen.sh` will check to see if one of the submodule files for example from `libglnx/` or from `bsdiff/` exists. + +Additionally, `autogen.sh` will check to see if the environment variable `NOCONFIGURE` is set. To run `./configure` manually, run autogen in a modified environment as such, `env NOCONFIGURE=1 ./autogen.sh`. + +Otherwise, leave `NOCONFIGURE` empty and `autogen.sh` will run `./configure` as part of the `autogen.sh` command when it executes. + +For more information on `--prefix` see [Variables for Installation Directories](https://www.gnu.org/prep/standards/html_node/Directory-Variables.html#Directory-Variables). + +`make install` will generate files for `/bin` and `/lib`. If `DESTDIR` is unspecified then OSTree will be installed in the default directory i.e. `/usr/local/bin` and its static libraries in `/usr/local/lib`. Note that the `/usr/local` portion of the path can be changed using the `--prefix` option for `./configure`. + +See this [GNU guide on `DESTDIR` Staged Installs](https://www.gnu.org/prep/standards/html_node/DESTDIR.html) for more information. + +#### Tip + +Make allows parallel execution of recipes. Use `make -j` to speed up the build. `` is typically `$((2 * $(nproc)))` for optimal performance, where `nproc` is the number of processing units (CPU cores) available. + +See page 106 of the [GNU Make Manual](https://www.gnu.org/software/make/manual/make.pdf) for more information about the `--jobs` or `-j` option. + +## [Testing a Build](#testing-a-build) + +It is best practice to build software (definitely including ostree) in a container or virtual machine first. + +### Testing in a Container + +There are a variety of container engines available and many distributions have pre-packaged versions of e.g. [Podman](https://github.com/projectatomic/libpod) and Docker. + +If you choose to use [Docker upstream](https://docs.docker.com/v17.09/engine/installation/linux/docker-ce/fedora/), you may want to follow this [post-installation guide for Docker](https://docs.docker.com/v17.09/engine/installation/linux/linux-postinstall/). This will allow you to run Docker as a non-root user on a Linux based host machine. + +You will need to have pushed a remote git branch `$REMOTE_BRANCH` (see `ostree-git.sh below`) in order to pull your changes into a container. + +The example below uses Docker to manage containers. Save the contents of this **Dockerfile** somewhere on your machine: + +```bash +# this pulls the fedora 28 image +FROM registry.fedoraproject.org/fedora:28 + +# install ostree dependencies +RUN dnf update -y && \ + dnf -y install @buildsys-build dnf-plugins-core && \ + dnf -y builddep ostree && \ + dnf clean all + +# clone ostree and update master branch +COPY ostree-git.sh / +RUN ../ostree-git.sh + +# builds ostree + any additional commands +COPY ostree-build.sh / + +# entry into the container will start at this directory +WORKDIR /ostree + +# run the following as `/bin/sh -c` +# or enter the container to execute ./ostree-build.sh +RUN ../ostree-build.sh + +``` + +Save the following bash scripts in the same directory as the Dockerfile. Then change the mode bit of these files so that they are executable, by running `chmod +x ostree-git.sh ostree-build.sh` + +```bash +#!/bin/bash + +# ostree-git.sh +# Clone ostree and update master branch + +set -euo pipefail + +# Set $USERNAME to your GitHub username here. +USERNAME="" + +# clone your fork of the OSTree repo, this will be in the "/" directory +git clone https://github.com/$USERNAME/ostree.git +cd ostree + +# Add upstream as remote and update master branch +git checkout master +git remote add upstream https://github.com/ostreedev/ostree.git +git pull --rebase upstream master +``` + +```bash +#!/bin/bash + +# ostree-build.sh +# Build and test OSTree + +set -euo pipefail + +# $REMOTE_BRANCH is the name of the remote branch in your +# repository that contains changes (e.g. my-patch). +REMOTE_BRANCH="" + +# fetch updates from origin +# origin url should be your forked ostree repository +git fetch origin + +# go to branch with changes +# if this branch already exists then checkout that branch +exit_code="$(git checkout --track origin/$REMOTE_BRANCH; echo $?)" +if [[ "$exit_code" == 1 ]] +then + echo "This branch:" $REMOTE_BRANCH "is not a remote branch." + exit +fi + +# make sure branch with changes is up-to-date +git pull origin $REMOTE_BRANCH + +# build OSTree commands for Fedora 28 and Fedora 28 Atomic Host +./autogen.sh --prefix=/usr --libdir=/usr/lib64 --sysconfdir=/etc +./configure --prefix=/usr +make -j$((2 * $(nproc))) +make install + +# any additional commands go here +``` + +**Build the container** + +Run `docker build` in the same directory of the `Dockerfile` like so: + +```bash +$ docker build -t ostree-fedora-test . +``` + +When this build is done, the `-t` option tags the image as `ostree-fedora-test`. + +**Note**: Do not forget the dot **.** at the end of the above command which specifies the location of the Dockerfile. + +You will see `ostree-fedora-test` listed when running `docker images`: + +```bash +REPOSITORY TAG IMAGE ID CREATED SIZE +ostree-fedora-test latest 817c04cc3656 1 day ago 978MB +``` + +**Entering the Container** + +To **start** the `ostree-fedora-test` container, run: + +```bash +$ docker run -it --rm --entrypoint /bin/sh --name ostree-testing ostree-fedora-test +``` + +**Note**: + +`--rm` option tells [Docker to automatically clean up the container and remove the file system when the container exits](https://docs.docker.com/engine/reference/run/#clean-up---rm). Otherwise remove it manually by running `docker rm `. + +The state of the container will not be saved when the shell prompt exits. Best practice is modify the Dockerfile to modify the image. + +**Testing in a Container Workflow** + +1. Edit the changes to OSTree on your local machine. +2. `git add` to stage the changed files, `git commit` and then `git push origin :`. +3. Testing on a _new_ container vs. Testing on an _existing_ container: + + If the `ostree-testing` container was newly built right after your changes have been committed, then the container's build of `ostree` should contain your changes. + + Else: Within the `ostree-testing` container, run `../ostree-build.sh` in the ostree directory. This will pull in changes from your branch and create a new `ostree` build. + +4. `make install` will install OSTree in the default location i.e. `/usr/..`in a Fedora 28 container. + +5. Test `ostree`. + +### Testing in a Virtual Machine + +To create a Fedora 28 Atomic Host Vagrant VM, run the following commands: + +```bash +$ mkdir atomic && cd atomic +$ vagrant init fedora/28-atomic-host && vagrant up +``` + +An option is to use `rsync` to transfer `ostree` files to a Vagrant VM. + +To find the IP address of a Vagrant VM, run `vagrant ssh-config` in the same directory as the `Vagrantfile`. + +**Steps to `rsync` files to test an `ostree` build**: + +1. Copy the contents of your public ssh key on your host machine e.g. `id_rsa.pub` to `/home/vagrant/.ssh/authorized_keys` on the VM. + +2. Run `sudo su`, followed by `ssh localhost` then press Ctrl+c to exit from the decision prompt. This will create the `.ssh` directory with the right permissions. + +3. Using `Vagrant` as the user, run `sudo cp ~/.ssh/authorized_keys /root/.ssh/`. So that user `root` has the the same login credentials. + +4. To override the `Read-only file system` warning, run `sudo ostree admin unlock`. + +5. `` will serve as the local install location for `ostree` and the path to this directory should be _absolute_ when specified in `DESTDIR`. + +6. Set `rsync` to sync changes in `/etc` and `/usr` from `/` on the host to the VM: + + ``` + $ rsync -av /etc/ root@:/etc + $ rsync -av /usr/ root@:/usr + ``` + + Using option `-n` will execute the commands as a trial, which is helpful to list the files that will be synced. + +7. Run the commands in step 6 each time a new `ostree` build is executed to update the change. Running `ls -lt` in the directory where the changed file is expected, is a simple way to check when a particular file was last modified. Proceed to the test changes `ostree` with the most recent changes. + +## Tutorial: Adding a basic builtin command to ostree + +### Modifying OSTree + +This will add a command which prints `Hello OSTree!` when `ostree hello-ostree` is entered. + +1. Create a file in `src/ostree` named `ot-builtin-hello-ostree.c`. Code that lives in here belongs to OSTree, and uses functionality from libostree. + +2. Add the following to `ot-builtin-hello-ostree.c`: + + #include "config.h" + + #include "ot-main.h" + #include "ot-builtins.h" + #include "ostree.h" + #include "otutil.h" + + // Structure for options such as ostree hello-ostree --option. + static GOptionEntry options[] = { + { NULL }, + }; + + gboolean + ostree_builtin_hello_ostree (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) + { + g_autoptr(GOptionContext) context = NULL; + g_autoptr(OstreeRepo) repo = NULL; + gboolean ret = FALSE; + + // Creates new command context, ready to be parsed. + // Input to g_option_context_new shows when running ostree --help + context = g_option_context_new (""); + + // Parses the command context according to the ostree CLI. + if (!ostree_option_context_parse (context, options, &argc, &argv, invocation, &repo, cancellable, error)) + goto out; + + g_print("Hello OSTree!\n"); + + ret = TRUE; + out: + return ret; + } + + This defines the functionality for `hello-ostree`. Now we have to make sure the CLI can refer to the execution function, and that autotools knows to build this file. + +3. Add the following in `src/ostree/main.c`: + + { "hello-ostree", // The name of the command + OSTREE_BUILTIN_FLAG_NO_REPO, // Flag not to require the `--repo` argument, see "ot-main.h" + ostree_builtin_hello_ostree, // Execution function for the command + "Print hello message" }, // Short description to appear when `ostree hello-ostree --help` is entered + +4. Add a macro for the function declaration of `ostree_builtin_hello_ostree`, in `ot-builtins.h`: + + BUILTINPROTO(hello_ostree); + + This makes the function definition visible to the CLI. + +5. Configure automake to include `ot-builtin-hello-ostree.c` in the build, by adding an entry in `Makefile-ostree.am` under `ostree_SOURCES`: + + src/ostree/ot-builtin-hello-ostree.c \ + +6. Rebuild ostree: + + $ make && make install DESTDIR=/path/to/install/the/content + +7. Execute the new `ostree` binary, from where you installed it: + + $ ostree hello-ostree + Hello OSTree! + +### [OSTree Tests](#ostree-tests) + +Tests for OSTree are done by shell scripting, by running OSTree commands and examining output. These steps will go through adding a test for `hello-ostree`. + +1. Create a file in `tests` called `test-hello-ostree.sh`. + +2. Add the following to `test-hello-ostree.sh`: + + set -euo pipefail # Ensure the test will not silently fail + + . $(dirname $0)/libtest.sh # Make libtest.sh functions available + + echo "1..1" # Declare which test is being run out of how many + + pushd ${test_tmpdir} + + ${CMD_PREFIX} ostree hello-ostree > hello-output.txt + assert_file_has_content hello-output.txt "Hello OSTree!" + + popd + + echo "ok hello ostree" # Indicate test success + + Many tests require a fake repository setting up (as most OSTree commands require `--repo` to be specified). See `test-pull-depth.sh` for an example of this setup. + +3. Configure automake to include `test-hello-ostree.sh` in the build, by adding an entry in `Makefile-tests.am` under `_installed_or_uninstalled_test_scripts`: + + tests/test-hello-ostree.sh \ + +4. Make sure `test-hello-ostree.sh` has executable permissions! + + $ chmod +x tests/test-hello-ostree.sh + +5. Run the test: + + $ make check TESTS="tests/test-hello-ostree.sh" + + Multiple tests can be specified: `make check TESTS="test1 test2 ..."`. To run all tests, use `make check`. + + Hopefully, the test passes! The following will be printed: + + PASS: tests/test-hello-ostree.sh 1 hello ostree + ============================================================================ + Testsuite summary for libostree 2018.8 + ============================================================================ + # TOTAL: 1 + # PASS: 1 + # SKIP: 0 + # XFAIL: 0 + # FAIL: 0 + # XPASS: 0 + # ERROR: 0 + ============================================================================ + +### Submitting a Patch + +After you have committed your changes and tested, you are ready to submit your patch! + +You should make sure your commits are placed on top of the latest changes from `upstream/master`: + +```bash +$ git pull --rebase upstream master +``` + +To submit your patch, open a pull request from your forked repository. Most often, you'll be merging into `ostree:master` from `:`. + +If some of your changes are complete and you would like feedback, you may also open a pull request that has WIP (Work In Progress) in the title. + +Before a pull request is considered merge ready, your commit messages should fall within the specified guideline. See [Commit message style](CONTRIBUTING.md#commit-message-style). + +See [CONTRIBUTING.md](CONTRIBUTING.md#submitting-patches) for information on squashing commits, and alternative options to submit patches. + +### Returning Workflow + +When returning to work on a patch, it is recommended to update your fork with the latest changes in the upstream master branch. + +If creating a new branch: + +``` +$ git checkout master +$ git pull upstream master +$ git checkout -b +``` + +If continuing on a branch already created: + +``` +$ git checkout +$ git pull --rebase upstream master +``` From 414891865568ee95978bfe2091ef6f8416726a1f Mon Sep 17 00:00:00 2001 From: bubblemelon <12985181+Bubblemelon@users.noreply.github.com> Date: Thu, 16 Aug 2018 17:00:04 -0400 Subject: [PATCH 32/36] docs: Add detail to CONTRIBUTING.md and link to tutorial This adds detailed information on commit message guidelines, a link to the contributing tutorial, and minor typo fixes. Closes: #1694 Approved by: cgwalters --- docs/CONTRIBUTING.md | 48 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index bbe0d553..ed22e90a 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -1,7 +1,7 @@ Submitting patches ------------------ -A majority of current maintainers prefer the Github pull request +A majority of current maintainers prefer the GitHub pull request model, and this motivated moving the primary git repository to . @@ -12,19 +12,19 @@ for more information. Instead, we use an instance of [Homu](https://github.com/servo/homu), currently known as `cgwalters-bot`. -As a review proceeds, the preferred method is to push `fixup!` -commits via `git commit --fixup`. Homu knows how to use -`--autosquash` when performing the final merge. See the +As a review proceeds, the preferred method is to push `fixup!` commits. Any commits committed with the `--fixup` option will have have the word `fixup!` in its commit title. This is to indicate that this particular commit will be squashed with the commit that was specified in this command, `git commit --fixup `. Homu knows how to use `--autosquash` when performing the final merge. + +See the [Git documentation](https://git-scm.com/docs/git-rebase) for more information. -Alternative methods if you don't like Github (also fully supported): +Alternative methods if you don't like GitHub (also fully supported): 1. Send mail to , with the patch attached 1. Attach them to It is likely however once a patch is ready to apply a maintainer -will push it to a github PR, and merge via Homu. +will push it to a GitHub PR, and merge via Homu. Commit message style -------------------- @@ -35,6 +35,37 @@ similar to the You may use `Signed-off-by`, but we're not requiring it. +**General Commit Message Guidelines**: + +1. Title + - Specify the context or category of the changes e.g. `lib` for library changes, `docs` for document changes, `bin/` for command changes, etc. + - Begin the title with the first letter of the first word capitalized. + - Aim for less than 50 characters, otherwise 72 characters max. + - Do not end the title with a period. + - Use an [imperative tone](https://en.wikipedia.org/wiki/Imperative_mood). +2. Body + - Separate the body with a blank line after the title. + - Begin a paragraph with the first letter of the first word capitalized. + - Each paragraph should be formatted within 72 characters. + - Content should be about what was changed and why this change was made. + - If your commit fixes an issue, the commit message should end with `Closes: #`. + +Commit Message example: + +```bash +: Less than 50 characters for subject title + +A paragraph of the body should be within 72 characters. + +This paragraph is also less than 72 characters. +``` + +For more information see [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/) + +**Editing a Committed Message:** + +To edit the message from the most recent commit run `git commit --amend`. To change older commits on the branch use `git rebase -i`. For a successful rebase have the branch track `upstream master`. Once the changes have been made and saved, run `git push --force origin `. + Running the test suite ---------------------- @@ -136,3 +167,8 @@ Instead do this: goto out; } } + +Contributing: Tutorial +---------------------- + +For a detailed walk-through on building, modifying, and testing, see this [tutorial on how to start contributing to OSTree](contributing-tutorial.md). \ No newline at end of file From d786b9fd5ffcee4a7c38caa1001b3009349e1a37 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 20 Aug 2018 11:17:29 -0400 Subject: [PATCH 33/36] man/config: Keep cmdsynopsis for GROUP.KEY version It's nice to still show the previous usage since it's easier to type in trivial cases like `core.mode`. Closes: #1710 Approved by: sinnykumari --- man/ostree-config.xml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/man/ostree-config.xml b/man/ostree-config.xml index f052aebb..dc180b83 100644 --- a/man/ostree-config.xml +++ b/man/ostree-config.xml @@ -50,9 +50,15 @@ Boston, MA 02111-1307, USA. + + ostree config get SECTIONNAME.KEYNAME + ostree config get --group=GROUPNAME KEYNAME + + ostree config set SECTIONNAME.KEYNAME VALUE + ostree config set --group=GROUPNAME KEYNAME VALUE @@ -68,7 +74,8 @@ Boston, MA 02111-1307, USA. Example - $ ostree config get --group=core mode + $ ostree config get core.mode bare + $ ostree config set --group='remote "myremote"' url http://example.com/repo From bb66a03fefb43cead6e059875cc5ad78a2401ec3 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 20 Aug 2018 11:17:57 -0400 Subject: [PATCH 34/36] ostree/config: Delete rogue printf and tweak help Minor tweak to the new `--group` flag help string. Also drop an extraneous `printf`. Closes: #1710 Approved by: sinnykumari --- src/ostree/ot-builtin-config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ostree/ot-builtin-config.c b/src/ostree/ot-builtin-config.c index b9fa824d..4368f50c 100644 --- a/src/ostree/ot-builtin-config.c +++ b/src/ostree/ot-builtin-config.c @@ -36,7 +36,7 @@ static char* opt_group; */ static GOptionEntry options[] = { - { "group", 0, 0, G_OPTION_ARG_STRING, &opt_group , "Group name followed by key for a remote config", NULL }, + { "group", 0, 0, G_OPTION_ARG_STRING, &opt_group , "Group name", NULL }, { NULL } }; @@ -88,7 +88,6 @@ ostree_builtin_config (int argc, char **argv, OstreeCommandInvocation *invocatio if (!strcmp (op, "set")) { - printf("GROUP NUMBER = %s %d\n", opt_group, argc); if (opt_group) { if (argc < 4) From 417b5c7067436893c865eabe1774777f30e98c59 Mon Sep 17 00:00:00 2001 From: Robert Fairley Date: Mon, 20 Aug 2018 15:40:32 -0400 Subject: [PATCH 35/36] docs: Add Contributing Tutorial to Mkdocs pages This adds the Contributing Tutorial (contributing-tutorial.md) to the pages setting of Mkdocs, so that the tutorial will render in the readthedocs.io documentation. Closes: #1711 Approved by: jlebon --- docs/CONTRIBUTING.md | 6 +++--- docs/contributing-tutorial.md | 18 ++++++++---------- mkdocs.yml | 1 + 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index ed22e90a..de14c380 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -168,7 +168,7 @@ Instead do this: } } -Contributing: Tutorial ----------------------- +Contributing Tutorial +--------------------- -For a detailed walk-through on building, modifying, and testing, see this [tutorial on how to start contributing to OSTree](contributing-tutorial.md). \ No newline at end of file +For a detailed walk-through on building, modifying, and testing, see this [tutorial on how to start contributing to OSTree](contributing-tutorial.md). diff --git a/docs/contributing-tutorial.md b/docs/contributing-tutorial.md index a1f0c4be..47d0a1e9 100644 --- a/docs/contributing-tutorial.md +++ b/docs/contributing-tutorial.md @@ -62,11 +62,11 @@ apt-get install build-essential && \ apt-get build-dep ostree ``` -[build.sh](../ci/build.sh) will have a list of packages needed to build ostree. +[build.sh](https://github.com/ostreedev/ostree/blob/master/ci/build.sh) will have a list of packages needed to build ostree. ### OSTree Build Commands -These are the basic commands to build OSTree. Depending on the OS that OSTree will be build for, the flags or options for `./autogen.sh` and `./configure` will vary. +These are the basic commands to build OSTree. Depending on the OS that OSTree will be built for, the flags or options for `./autogen.sh` and `./configure` will vary. See `ostree-build.sh` in this tutorial below for specific commands to building OSTree for Fedora 28 and Fedora 28 Atomic Host. @@ -103,7 +103,7 @@ Make allows parallel execution of recipes. Use `make -j` to speed up the buil See page 106 of the [GNU Make Manual](https://www.gnu.org/software/make/manual/make.pdf) for more information about the `--jobs` or `-j` option. -## [Testing a Build](#testing-a-build) +## Testing a Build It is best practice to build software (definitely including ostree) in a container or virtual machine first. @@ -277,10 +277,8 @@ To find the IP address of a Vagrant VM, run `vagrant ssh-config` in the same dir 6. Set `rsync` to sync changes in `/etc` and `/usr` from `/` on the host to the VM: - ``` - $ rsync -av /etc/ root@:/etc - $ rsync -av /usr/ root@:/usr - ``` + $ rsync -av /etc/ root@:/etc + $ rsync -av /usr/ root@:/usr Using option `-n` will execute the commands as a trial, which is helpful to list the files that will be synced. @@ -358,7 +356,7 @@ This will add a command which prints `Hello OSTree!` when `ostree hello-ostree` $ ostree hello-ostree Hello OSTree! -### [OSTree Tests](#ostree-tests) +### OSTree Tests Tests for OSTree are done by shell scripting, by running OSTree commands and examining output. These steps will go through adding a test for `hello-ostree`. @@ -436,7 +434,7 @@ When returning to work on a patch, it is recommended to update your fork with th If creating a new branch: -``` +```bash $ git checkout master $ git pull upstream master $ git checkout -b @@ -444,7 +442,7 @@ $ git checkout -b If continuing on a branch already created: -``` +```bash $ git checkout $ git pull --rebase upstream master ``` diff --git a/mkdocs.yml b/mkdocs.yml index 3b882e1c..f4c73c35 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -2,6 +2,7 @@ site_name: OSTree pages: - Home: 'index.md' - Contributing: 'CONTRIBUTING.md' + - Contributing Tutorial: 'contributing-tutorial.md' - Manual: - Introduction: 'manual/introduction.md' - Repository: 'manual/repo.md' From 7aa242c34cbfde2cd09d36d1ee2b32166287d9b6 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 14 Aug 2018 19:45:32 +0000 Subject: [PATCH 36/36] Release 2018.8 Closes: #1705 Approved by: jlebon --- configure.ac | 2 +- src/libostree/libostree-devel.sym | 2 +- src/libostree/libostree-released.sym | 2 ++ tests/test-symbols.sh | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 238f51cd..4ccd79c6 100644 --- a/configure.ac +++ b/configure.ac @@ -7,7 +7,7 @@ m4_define([year_version], [2018]) m4_define([release_version], [8]) m4_define([package_version], [year_version.release_version]) AC_INIT([libostree], [package_version], [walters@verbum.org]) -is_release_build=no +is_release_build=yes AC_CONFIG_HEADER([config.h]) AC_CONFIG_MACRO_DIR([buildutil]) AC_CONFIG_AUX_DIR([build-aux]) diff --git a/src/libostree/libostree-devel.sym b/src/libostree/libostree-devel.sym index 9928ac4f..24db9df2 100644 --- a/src/libostree/libostree-devel.sym +++ b/src/libostree/libostree-devel.sym @@ -18,7 +18,7 @@ ***/ /* Add new symbols here. Release commits should copy this section into -released.sym. */ -LIBOSTREE_2018.8 { +LIBOSTREE_2018.9 { } LIBOSTREE_2018.7; /* Stub section for the stable release *after* this development one; don't diff --git a/src/libostree/libostree-released.sym b/src/libostree/libostree-released.sym index 1e17e581..ae3bb5ed 100644 --- a/src/libostree/libostree-released.sym +++ b/src/libostree/libostree-released.sym @@ -534,6 +534,8 @@ global: ostree_mutable_tree_check_error; } LIBOSTREE_2018.6; +/* No new symbols in 2018.8 */ + /* NOTE: Only add more content here in release commits! See the * comments at the top of this file. */ diff --git a/tests/test-symbols.sh b/tests/test-symbols.sh index faf58dcf..df7f8648 100755 --- a/tests/test-symbols.sh +++ b/tests/test-symbols.sh @@ -54,7 +54,7 @@ echo 'ok documented symbols' # ONLY update this checksum in release commits! cat > released-sha256.txt <