From 758be138bec6026f5ff166f03463e5329fded4b9 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 3 Nov 2017 15:29:26 -0400 Subject: [PATCH 01/46] build-sys: Post-release version bump Closes: #1324 Approved by: pwithnall --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 92248af8..ea74a9ef 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], [2017]) -m4_define([release_version], [13]) +m4_define([release_version], [14]) 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]) From 6ec4cd3eb56edfbc204877862ba51a47672d124c Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 3 Nov 2017 15:47:16 +0000 Subject: [PATCH 02/46] build: Ensure enumtypes.h is built before enumtypes.c ostree-enumtypes.c includes ostree-enumtypes.h, so make needs to be told about the dependency. Without it, parallel make could try to build ostree-enumtypes.c before the header file exists. I hit this when running `make -j OSTree-1.0.gir`. Closes: #1322 Approved by: pwithnall --- Makefile-libostree.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile-libostree.am b/Makefile-libostree.am index e2ebae3a..f8376096 100644 --- a/Makefile-libostree.am +++ b/Makefile-libostree.am @@ -54,7 +54,7 @@ src/libostree/ostree-enumtypes.h: src/libostree/ostree-enumtypes.h.template $(EN --template $< \ $(ENUM_TYPES) > $@.tmp && mv $@.tmp $@ -src/libostree/ostree-enumtypes.c: src/libostree/ostree-enumtypes.c.template $(ENUM_TYPES) +src/libostree/ostree-enumtypes.c: src/libostree/ostree-enumtypes.c.template src/libostree/ostree-enumtypes.h $(ENUM_TYPES) $(AM_V_GEN) $(GLIB_MKENUMS) \ --template $< \ $(ENUM_TYPES) > $@.tmp && mv $@.tmp $@ From 69c4737491a3cdb484eaa932f2fa5cb2153377f5 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Mon, 16 Oct 2017 15:51:40 +0000 Subject: [PATCH 03/46] build: Define OSTREE_ENABLE_EXPERIMENTAL_API for g-ir-scanner When compiling libostree, OSTREE_ENABLE_EXPERIMENTAL_API is managed via config.h. However, g-ir-scanner can't use that since it gets confused about the namespace of all the random macros. It won't include the experimental APIs unless the macro is defined through another means. Without this, none of the experimental APIs were being included in the gir data. Closes: #1322 Approved by: pwithnall --- Makefile-libostree.am | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Makefile-libostree.am b/Makefile-libostree.am index f8376096..63b0038b 100644 --- a/Makefile-libostree.am +++ b/Makefile-libostree.am @@ -261,6 +261,10 @@ OSTree-1.0.gir: libostree-1.la Makefile OSTree_1_0_gir_EXPORT_PACKAGES = ostree-1 OSTree_1_0_gir_INCLUDES = Gio-2.0 OSTree_1_0_gir_CFLAGS = $(libostree_1_la_CFLAGS) +if ENABLE_EXPERIMENTAL_API +# When compiling this is set via config.h, but g-ir-scanner can't use that +OSTree_1_0_gir_CFLAGS += -DOSTREE_ENABLE_EXPERIMENTAL_API=1 +endif OSTree_1_0_gir_LIBS = libostree-1.la OSTree_1_0_gir_SCANNERFLAGS = --warn-all --identifier-prefix=Ostree --symbol-prefix=ostree $(GI_SCANNERFLAGS) OSTree_1_0_gir_FILES = $(libostreeinclude_HEADERS) $(filter-out %-private.h %/ostree-soup-uri.h $(libostree_experimental_headers),$(libostree_1_la_SOURCES)) From 03bbe4553007a6d0b92efc35e2fda3d6c90bc6ab Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 3 Nov 2017 15:53:19 +0000 Subject: [PATCH 04/46] lib/core: Fix documentation comment in ostree_validate_collection_id g-ir-scanner was spitting this warning: src/libostree/ostree-core.c:281: Warning: OSTree: ostree_validate_collection_id: unknown parameter 'rev' in documentation comment, should be 'collection_id' Closes: #1322 Approved by: pwithnall --- src/libostree/ostree-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c index cee036d8..ea4c6cc3 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -278,7 +278,7 @@ ostree_validate_remote_name (const char *remote_name, /** * ostree_validate_collection_id: - * @rev: (nullable): A collection ID + * @collection_id: (nullable): A collection ID * @error: Error * * Check whether the given @collection_id is valid. Return an error if it is From 519b30b7e1979fea827ea4fe9b0e9ac4db99d631 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 3 Nov 2017 16:10:23 +0000 Subject: [PATCH 05/46] lib/pull: Skip ostree_repo_resolve_keyring_for_collection for bindings Since ostree_remote_get_type is not made available to g-ir-scanner, it treats OstreeRemote as a bare struct. That's not kosher for bindings and it issues the following warning: src/libostree/ostree-repo-pull.c:5560: Warning: OSTree: ostree_repo_resolve_keyring_for_collection: return value: Invalid non-constant return of bare structure or union; register as boxed type or (skip) For now, just skip this API for bindings. Closes: #1322 Approved by: pwithnall --- 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 44fae35e..20fa8277 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -5539,8 +5539,11 @@ check_remote_matches_collection_id (OstreeRepo *repo, return g_str_equal (remote_collection_id, collection_id); } +/* FIXME: Export this to bindings once OstreeRemote is properly registered as + * a boxed type. + */ /** - * ostree_repo_resolve_keyring_for_collection: + * ostree_repo_resolve_keyring_for_collection: (skip) * @self: an #OstreeRepo * @collection_id: the collection ID to look up a keyring for * @cancellable: (nullable): a #GCancellable, or %NULL From 2e9fea8fa54f431a6e4cc2993d0b4b2ca18a6070 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 3 Nov 2017 20:13:50 +0000 Subject: [PATCH 06/46] tests: Don't symlink rofiles-fuse if it's disabled Creating the symlink will cause make to try to build rofiles-fuse, which will fail if it's disabled. Normally I wouldn't disable rofiles-fuse, but it's triggering a hang in our ARM Xenial builder's kernel in splice. I'm sure that's fuse's fault, but for now I just need to disable rofiles-fuse there and found --disable-rofiles-fuse didn't actually work. Closes: #1325 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 bc962aac..164717b1 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -41,7 +41,7 @@ AM_TESTS_ENVIRONMENT += OT_SKIP_READDIR_RAND=1 G_SLICE=always-malloc endif uninstalled_test_data = tests/ostree-symlink-stamp tests/ostree-prepare-root-symlink-stamp \ - tests/ostree-remount-symlink-stamp tests/rofiles-fuse-symlink-stamp + tests/ostree-remount-symlink-stamp dist_uninstalled_test_scripts = tests/test-symbols.sh tests/coccinelle.sh @@ -143,6 +143,7 @@ endif if BUILDOPT_FUSE _installed_or_uninstalled_test_scripts += tests/test-rofiles-fuse.sh +uninstalled_test_data += tests/rofiles-fuse-symlink-stamp else EXTRA_DIST += tests/test-rofiles-fuse.sh endif From 7296bf3dcc190d6d137790ff3f40f147e6ef742f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 2 Nov 2017 11:50:12 -0400 Subject: [PATCH 07/46] build: Add -Werror=undef by default, fix fallout The main thing here is that a ton of stuff has happened in gnulib since we imported `parse-datetime.y`. I cherry-picked a little bit of it, but that upstream doesn't seem to build with `-Wundef`, so I just deleted some hunks. (Note I reindented the warnings consistently) Update submodule: libglnx Closes: #1320 Approved by: jlebon --- configure.ac | 38 +++++++------ libglnx | 2 +- src/libostree/ostree-enumtypes.c.template | 4 +- src/ostree/parse-datetime.y | 67 ++++------------------- 4 files changed, 34 insertions(+), 77 deletions(-) diff --git a/configure.ac b/configure.ac index ea74a9ef..6b095504 100644 --- a/configure.ac +++ b/configure.ac @@ -30,23 +30,24 @@ AC_SUBST([PACKAGE_VERSION], [package_version]) AS_IF([echo "$CFLAGS" | grep -q -E -e '-Werror($| )'], [], [ CC_CHECK_FLAGS_APPEND([WARN_CFLAGS], [CFLAGS], [\ - -pipe \ - -Wall \ - -Werror=empty-body \ - -Werror=strict-prototypes \ - -Werror=missing-prototypes \ - -Werror=implicit-function-declaration \ - "-Werror=format=2 -Werror=format-security -Werror=format-nonliteral" \ - -Werror=pointer-arith -Werror=init-self \ - -Werror=missing-declarations \ - -Werror=return-type \ - -Werror=switch \ - -Werror=overflow \ - -Werror=int-conversion \ - -Werror=parenthesis \ - -Werror=incompatible-pointer-types \ - -Werror=misleading-indentation \ - -Werror=missing-include-dirs -Werror=aggregate-return \ + -pipe \ + -Wall \ + -Werror=empty-body \ + -Werror=strict-prototypes \ + -Werror=missing-prototypes \ + -Werror=implicit-function-declaration \ + "-Werror=format=2 -Werror=format-security -Werror=format-nonliteral" \ + -Werror=pointer-arith -Werror=init-self \ + -Werror=missing-declarations \ + -Werror=return-type \ + -Werror=switch \ + -Werror=overflow \ + -Werror=int-conversion \ + -Werror=parenthesis \ + -Werror=undef \ + -Werror=incompatible-pointer-types \ + -Werror=misleading-indentation \ + -Werror=missing-include-dirs -Werror=aggregate-return \ -Werror=unused-result \ ])]) AC_SUBST(WARN_CFLAGS) @@ -83,6 +84,9 @@ AC_SUBST([OSTREE_FEATURES]) GLIB_TESTS LIBGLNX_CONFIGURE +dnl These bits attempt to mirror https://github.com/coreutils/gnulib/blob/e369b04cca4da1534c98628b8ee4648bfca2bb3a/m4/parse-datetime.m4#L27 +AC_CHECK_FUNCS([nanotime clock_gettime]) +AC_STRUCT_TIMEZONE AC_CHECK_HEADER([sys/xattr.h],,[AC_MSG_ERROR([You must have sys/xattr.h from glibc])]) AS_IF([test "$YACC" != "bison -y"], [AC_MSG_ERROR([bison not found but required])]) diff --git a/libglnx b/libglnx index d15a3790..b36606b3 160000 --- a/libglnx +++ b/libglnx @@ -1 +1 @@ -Subproject commit d15a3790074fd982f2611a5b450dea61052dfc0b +Subproject commit b36606b366d39c7ddb90ee21d622c0cb1da118ed diff --git a/src/libostree/ostree-enumtypes.c.template b/src/libostree/ostree-enumtypes.c.template index f7eecf24..751c458a 100644 --- a/src/libostree/ostree-enumtypes.c.template +++ b/src/libostree/ostree-enumtypes.c.template @@ -18,9 +18,7 @@ * Boston, MA 02111-1307, USA. */ -#ifndef _GNU_SOURCE -#define _GNU_SOURCE -#endif +#include "config.h" #include #include "ostree-enumtypes.h" diff --git a/src/ostree/parse-datetime.y b/src/ostree/parse-datetime.y index 50917354..e1ce3057 100644 --- a/src/ostree/parse-datetime.y +++ b/src/ostree/parse-datetime.y @@ -38,6 +38,7 @@ #include #include #include +#include /* There's no need to extend the stack, so there's no need to involve alloca. */ @@ -54,11 +55,11 @@ xmemdup (void const *p, size_t s) static void gettime (struct timespec *ts) { -#if HAVE_NANOTIME +#ifdef HAVE_NANOTIME nanotime (ts); #else -# if defined CLOCK_REALTIME && HAVE_CLOCK_GETTIME +# if defined(CLOCK_REALTIME) && defined(HAVE_CLOCK_GETTIME) if (clock_gettime (CLOCK_REALTIME, ts) == 0) return; # endif @@ -134,13 +135,6 @@ gettime (struct timespec *ts) #define HOUR(x) ((x) * 60) -/* long_time_t is a signed integer type that contains all time_t values. */ -#if TIME_T_FITS_IN_LONG_INT -typedef long int long_time_t; -#else -typedef time_t long_time_t; -#endif - /* Convert a possibly-signed character to an unsigned character. This is a bit safer than casting to unsigned char, since it catches some type errors that the cast doesn't. */ @@ -180,15 +174,11 @@ typedef struct long int day; long int hour; long int minutes; - long_time_t seconds; - long int ns; + intmax_t seconds; + int ns; } relative_time; -#if HAVE_COMPOUND_LITERALS -# define RELATIVE_TIME_0 ((relative_time) { 0, 0, 0, 0, 0, 0, 0 }) -#else -static relative_time const RELATIVE_TIME_0; -#endif +#define RELATIVE_TIME_0 ((relative_time) { 0, 0, 0, 0, 0, 0, 0 }) /* Information passed to and from the parser. */ typedef struct @@ -955,7 +945,8 @@ lookup_zone (parser_control const *pc, char const *name) return NULL; } -#if ! HAVE_TM_GMTOFF +// #if ! HAVE_TM_GMTOFF +#if 1 // Always true for us /* Yield the difference between *A and *B, measured in seconds, ignoring leap seconds. The body of this function is taken directly from the GNU C Library; @@ -1580,10 +1571,10 @@ parse_datetime (struct timespec *result, char const *p, time_t t1 = t0 + d1; long int d2 = 60 * pc.rel.minutes; time_t t2 = t1 + d2; - long_time_t d3 = pc.rel.seconds; - long_time_t t3 = t2 + d3; + intmax_t d3 = pc.rel.seconds; + intmax_t t3 = t2 + d3; long int d4 = (sum_ns - normalized_ns) / BILLION; - long_time_t t4 = t3 + d4; + intmax_t t4 = t3 + d4; time_t t5 = t4; if ((d1 / (60 * 60) ^ pc.rel.hour) @@ -1611,39 +1602,3 @@ parse_datetime (struct timespec *result, char const *p, free (tz0); return ok; } - -#if TEST - -int -main (int ac, char **av) -{ - char buff[BUFSIZ]; - - printf ("Enter date, or blank line to exit.\n\t> "); - fflush (stdout); - - buff[BUFSIZ - 1] = '\0'; - while (fgets (buff, BUFSIZ - 1, stdin) && buff[0]) - { - struct timespec d; - struct tm const *tm; - if (! parse_datetime (&d, buff, NULL)) - printf ("Bad format - couldn't convert.\n"); - else if (! (tm = localtime (&d.tv_sec))) - { - long int sec = d.tv_sec; - printf ("localtime (%ld) failed\n", sec); - } - else - { - int ns = d.tv_nsec; - printf ("%04ld-%02d-%02d %02d:%02d:%02d.%09d\n", - tm->tm_year + 1900L, tm->tm_mon + 1, tm->tm_mday, - tm->tm_hour, tm->tm_min, tm->tm_sec, ns); - } - printf ("\t> "); - fflush (stdout); - } - return 0; -} -#endif /* TEST */ From 015513b8f979ab0769ad36da868544378e553041 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 3 Nov 2017 18:43:00 +0000 Subject: [PATCH 08/46] lib/pull: Avoid error if current with --require-static-deltas A tricky thing here that caused this to go past a lot of our tests is that the code was mostly OK if there was an available delta from an older commit. But this case broke if we e.g. had a new OS deployment and did a `--require-static-deltas` pull, i.e. the initial state. I cleaned up our "find static delta state" function to return an enumeration, and extended it with an "already have the commit" state. A problem I then hit is that we've historically fetched detached metadata for non-delta pulls, even if the commit hasn't changed. I decided not to do that for `--require-static-deltas` pulls for now; otherwise the code gets notably more complex. Closes: https://github.com/ostreedev/ostree/issues/1321 Closes: #1323 Approved by: jlebon --- src/libostree/ostree-repo-pull.c | 119 +++++++++++++++++++++++++------ tests/pull-test.sh | 6 ++ 2 files changed, 102 insertions(+), 23 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index 20fa8277..b4310027 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -2325,19 +2325,39 @@ process_one_static_delta (OtPullData *pull_data, return ret; } -/* Loop over the static delta data we got from the summary, - * and find the newest commit for @out_from_revision that - * goes to @to_revision. +/* + * DELTA_SEARCH_RESULT_UNCHANGED: + * We already have the commit. * - * Additionally, @out_have_scratch_delta will be set to %TRUE - * if there is a %NULL → @to_revision delta, also known as + * DELTA_SEARCH_RESULT_NO_MATCH: + * No deltas were found. + * + * DELTA_SEARCH_RESULT_FROM: + * A regular delta was found, and the "from" revision will be + * set in `from_revision`. + * + * DELTA_SEARCH_RESULT_SCRATCH: + * There is a %NULL → @to_revision delta, also known as * a "from scratch" delta. */ +typedef struct { + enum { + DELTA_SEARCH_RESULT_UNCHANGED, + DELTA_SEARCH_RESULT_NO_MATCH, + DELTA_SEARCH_RESULT_FROM, + DELTA_SEARCH_RESULT_SCRATCH, + } result; + char from_revision[OSTREE_SHA256_STRING_LEN+1]; +} DeltaSearchResult; + +/* Loop over the static delta data we got from the summary, + * and find the a delta path (if available) that goes to @to_revision. + * See the enum in `DeltaSearchResult` for available result types. + */ static gboolean get_best_static_delta_start_for (OtPullData *pull_data, const char *to_revision, - gboolean *out_have_scratch_delta, - char **out_from_revision, + DeltaSearchResult *out_result, GCancellable *cancellable, GError **error) { @@ -2348,7 +2368,28 @@ get_best_static_delta_start_for (OtPullData *pull_data, g_assert (pull_data->summary_deltas_checksums != NULL); - *out_have_scratch_delta = FALSE; + out_result->result = DELTA_SEARCH_RESULT_NO_MATCH; + out_result->from_revision[0] = '\0'; + + /* First, do we already have this commit completely downloaded? */ + gboolean have_to_rev; + if (!ostree_repo_has_object (pull_data->repo, OSTREE_OBJECT_TYPE_COMMIT, + to_revision, &have_to_rev, + cancellable, error)) + return FALSE; + if (have_to_rev) + { + OstreeRepoCommitState to_rev_state; + if (!ostree_repo_load_commit (pull_data->repo, to_revision, + NULL, &to_rev_state, error)) + return FALSE; + if (!(to_rev_state & OSTREE_REPO_COMMIT_STATE_PARTIAL)) + { + /* We already have this commit, we're done! */ + out_result->result = DELTA_SEARCH_RESULT_UNCHANGED; + return TRUE; /* Early return */ + } + } /* Loop over all deltas known from the summary file, * finding ones which go to to_revision */ @@ -2366,9 +2407,17 @@ get_best_static_delta_start_for (OtPullData *pull_data, continue; if (cur_from_rev) - g_ptr_array_add (candidates, g_steal_pointer (&cur_from_rev)); + { + g_ptr_array_add (candidates, g_steal_pointer (&cur_from_rev)); + } else - *out_have_scratch_delta = TRUE; + { + /* We note that we have a _SCRATCH delta here, but we'll prefer using + * "from" deltas (obviously, they'll be smaller) where possible if we + * find one below. + */ + out_result->result = DELTA_SEARCH_RESULT_SCRATCH; + } } /* Loop over our candidates, find the newest one */ @@ -2407,7 +2456,11 @@ get_best_static_delta_start_for (OtPullData *pull_data, } } - *out_from_revision = g_strdup (newest_candidate); + if (newest_candidate) + { + out_result->result = DELTA_SEARCH_RESULT_FROM; + memcpy (out_result->from_revision, newest_candidate, OSTREE_SHA256_STRING_LEN+1); + } return TRUE; } @@ -3082,25 +3135,45 @@ initiate_request (OtPullData *pull_data, /* If we have a summary, we can use the newer logic */ if (pull_data->summary) { - gboolean have_scratch_delta = FALSE; + DeltaSearchResult deltares; /* Look for a delta to @to_revision in the summary data */ - if (!get_best_static_delta_start_for (pull_data, to_revision, - &have_scratch_delta, &delta_from_revision, + if (!get_best_static_delta_start_for (pull_data, to_revision, &deltares, pull_data->cancellable, error)) return FALSE; - if (delta_from_revision) /* Did we find a delta FROM commit? */ - initiate_delta_request (pull_data, delta_from_revision, to_revision, ref); - else if (have_scratch_delta) /* No delta FROM, do we have a scratch? */ - initiate_delta_request (pull_data, NULL, to_revision, ref); - else if (pull_data->require_static_deltas) /* No deltas found; are they required? */ + switch (deltares.result) { - set_required_deltas_error (error, (ref != NULL) ? ref->ref_name : "", to_revision); - return FALSE; + case DELTA_SEARCH_RESULT_NO_MATCH: + { + if (pull_data->require_static_deltas) /* No deltas found; are they required? */ + { + set_required_deltas_error (error, (ref != NULL) ? ref->ref_name : "", to_revision); + return FALSE; + } + else /* No deltas, fall back to object fetches. */ + queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); + } + break; + case DELTA_SEARCH_RESULT_FROM: + initiate_delta_request (pull_data, deltares.from_revision, to_revision, ref); + break; + case DELTA_SEARCH_RESULT_SCRATCH: + initiate_delta_request (pull_data, NULL, to_revision, ref); + break; + case DELTA_SEARCH_RESULT_UNCHANGED: + { + /* If we already have the commit, here things get a little special; we've historically + * fetched detached metadata, so let's keep doing that. But in the --require-static-deltas + * path, we don't, under the assumption the user wants as little network traffic as + * possible. + */ + if (pull_data->require_static_deltas) + break; + else + queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); + } } - else /* No deltas, fall back to object fetches. */ - queue_scan_one_metadata_object (pull_data, to_revision, OSTREE_OBJECT_TYPE_COMMIT, NULL, 0, ref); } else if (ref != NULL) { diff --git a/tests/pull-test.sh b/tests/pull-test.sh index 3f8030e0..c09feb30 100644 --- a/tests/pull-test.sh +++ b/tests/pull-test.sh @@ -381,6 +381,12 @@ fi ${CMD_PREFIX} ostree --repo=repo fsck done +# Test no-op with deltas: https://github.com/ostreedev/ostree/issues/1321 +cd ${test_tmpdir} +repo_init --no-gpg-verify +${CMD_PREFIX} ostree --repo=repo pull origin main +${CMD_PREFIX} ostree --repo=repo pull --require-static-deltas origin main + cd ${test_tmpdir} repo_init --no-gpg-verify ${CMD_PREFIX} ostree --repo=repo pull origin main@${prev_rev} From a16dddcaff5eb26f4843b89cd48213f1dd4817bd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 6 Nov 2017 14:37:24 -0500 Subject: [PATCH 09/46] build: Work around -Wundef with older GLib This previous PR caused our `-Werror=undef` to trigger when building with older GLib; this should work around it. Closes: #1327 Approved by: jlebon --- Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index 2a33282f..e1dfd32b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -29,7 +29,7 @@ AM_CPPFLAGS += -DDATADIR='"$(datadir)"' -DLIBEXECDIR='"$(libexecdir)"' \ -DOSTREE_COMPILATION \ -DG_LOG_DOMAIN=\"OSTree\" \ -DOSTREE_GITREV='"$(OSTREE_GITREV)"' \ - -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_50 \ + -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 '-DGLIB_VERSION_MAX_ALLOWED=G_ENCODE_VERSION(2,50)' \ -DSOUP_VERSION_MIN_REQUIRED=SOUP_VERSION_2_40 -DSOUP_VERSION_MAX_ALLOWED=SOUP_VERSION_2_48 AM_CFLAGS += -std=gnu99 $(WARN_CFLAGS) AM_DISTCHECK_CONFIGURE_FLAGS += \ From 9f4b5c0cc5abc255846b63a20de2082706c77aba Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 6 Nov 2017 15:42:05 -0500 Subject: [PATCH 10/46] build: Also fix -Werror=undef for old libsoup Same change as before, but for libsoup. Closes: #1328 Approved by: jlebon --- Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile.am b/Makefile.am index e1dfd32b..6043b2aa 100644 --- a/Makefile.am +++ b/Makefile.am @@ -30,7 +30,7 @@ AM_CPPFLAGS += -DDATADIR='"$(datadir)"' -DLIBEXECDIR='"$(libexecdir)"' \ -DG_LOG_DOMAIN=\"OSTree\" \ -DOSTREE_GITREV='"$(OSTREE_GITREV)"' \ -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_40 '-DGLIB_VERSION_MAX_ALLOWED=G_ENCODE_VERSION(2,50)' \ - -DSOUP_VERSION_MIN_REQUIRED=SOUP_VERSION_2_40 -DSOUP_VERSION_MAX_ALLOWED=SOUP_VERSION_2_48 + -DSOUP_VERSION_MIN_REQUIRED=SOUP_VERSION_2_40 '-DSOUP_VERSION_MAX_ALLOWED=G_ENCODE_VERSION(2,48)' AM_CFLAGS += -std=gnu99 $(WARN_CFLAGS) AM_DISTCHECK_CONFIGURE_FLAGS += \ --enable-gtk-doc \ From 7d2ad351c42c9f4bf3d0d8f88815458e84d9677e Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 7 Nov 2017 14:53:12 +0000 Subject: [PATCH 11/46] build: Add a TODO comment about improving glib-mkenums usage in future Signed-off-by: Philip Withnall https://github.com/ostreedev/ostree/pull/1329 Closes: #1330 Approved by: cgwalters --- Makefile-libostree.am | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile-libostree.am b/Makefile-libostree.am index 63b0038b..39dc0d14 100644 --- a/Makefile-libostree.am +++ b/Makefile-libostree.am @@ -49,6 +49,7 @@ libostreeinclude_HEADERS = $(libostree_public_headers) $(libostree_public_built_ ENUM_TYPES = $(NULL) ENUM_TYPES += $(srcdir)/src/libostree/ostree-fetcher.h +# TODO: GLIB_CHECK_VERSION > 2.5x: use --output instead of mv (see https://github.com/ostreedev/ostree/pull/1329) src/libostree/ostree-enumtypes.h: src/libostree/ostree-enumtypes.h.template $(ENUM_TYPES) $(AM_V_GEN) $(GLIB_MKENUMS) \ --template $< \ From 176a7b4778fbacbbb0824cf8c612656cd0985499 Mon Sep 17 00:00:00 2001 From: Kalev Lember Date: Tue, 7 Nov 2017 16:10:45 +0100 Subject: [PATCH 12/46] fetcher/curl: Fix invalid memory access in finalize() Reorder cleanup functions so that curl_multi_cleanup() runs before self->sockets is destroyed. This avoids an assert and invalid memory access in sock_cb where self->sockets is dereferenced during curl_multi_cleanup(). Closes: https://github.com/ostreedev/ostree/issues/1331 Closes: #1332 Approved by: cgwalters --- src/libostree/ostree-fetcher-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-fetcher-curl.c b/src/libostree/ostree-fetcher-curl.c index 1f641882..8a23b163 100644 --- a/src/libostree/ostree-fetcher-curl.c +++ b/src/libostree/ostree-fetcher-curl.c @@ -167,6 +167,7 @@ _ostree_fetcher_finalize (GObject *object) { OstreeFetcher *self = OSTREE_FETCHER (object); + curl_multi_cleanup (self->multi); g_free (self->remote_name); g_free (self->cookie_jar_path); g_free (self->proxy); @@ -177,7 +178,6 @@ _ostree_fetcher_finalize (GObject *object) g_clear_pointer (&self->timer_event, (GDestroyNotify)destroy_and_unref_source); if (self->mainctx) g_main_context_unref (self->mainctx); - curl_multi_cleanup (self->multi); G_OBJECT_CLASS (_ostree_fetcher_parent_class)->finalize (object); } From 9c4870b5e1512e2e8ceb2be913c7e93f0999462a Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 8 Nov 2017 15:10:31 +0000 Subject: [PATCH 13/46] lib/repo: Add OSTREE_REPO_COMMIT_STATE_NORMAL to represent most commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows more explicit handling of commit state in code using libostree, rather than hard-coding a commit state of 0 for ‘normal’. Signed-off-by: Philip Withnall Closes: #1335 Approved by: cgwalters --- src/libostree/ostree-repo.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index 15e5f94e..9a1b65ae 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -540,6 +540,7 @@ gboolean ostree_repo_load_variant_if_exists (OstreeRepo *self, GError **error); typedef enum { + OSTREE_REPO_COMMIT_STATE_NORMAL = 0, OSTREE_REPO_COMMIT_STATE_PARTIAL = (1 << 0), } OstreeRepoCommitState; From 3cf53f7c58540ac9ea0ac3d1a2c3fe548ba0907d Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Wed, 8 Nov 2017 15:11:25 +0000 Subject: [PATCH 14/46] lib/repo: Add gtk-doc comment to OstreeRepoCommitState Signed-off-by: Philip Withnall Closes: #1335 Approved by: cgwalters --- src/libostree/ostree-repo.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index 9a1b65ae..bec43c30 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -539,6 +539,18 @@ gboolean ostree_repo_load_variant_if_exists (OstreeRepo *self, GVariant **out_variant, GError **error); +/** + * OstreeRepoCommitState: + * @OSTREE_REPO_COMMIT_STATE_NORMAL: Commit is complete. This is the default. + * (Since: 2017.14.) + * @OSTREE_REPO_COMMIT_STATE_PARTIAL: One or more objects are missing from the + * local copy of the commit, but metadata is present. (Since: 2015.7.) + * + * Flags representing the state of a commit in the local repository, as returned + * by ostree_repo_load_commit(). + * + * Since: 2015.7 + */ typedef enum { OSTREE_REPO_COMMIT_STATE_NORMAL = 0, OSTREE_REPO_COMMIT_STATE_PARTIAL = (1 << 0), From 9856ed38404b709877d94a4c3b7996be7a4fe84f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 7 Nov 2017 17:52:12 -0500 Subject: [PATCH 15/46] deltas: Don't try to rollsum/bsdiff .xz files Fedora switched to 'xz' compress kernel modules, and recently [RHEL7 did too](https://bugzilla.redhat.com/show_bug.cgi?id=1367496). This compression defeats bsdiff. While we have a "rollsum-able" test, we don't have a "bsdiff-able" test as it'd be very expensive (we'd have to bsdiff, then apply it and compare the result). Let's do the tactical quick fix here and just not try to delta files ending in `.xz.`. This avoids us using bsdiff pointlessly for over 4000 files, which is quite a notable speed increase for generating deltas. Closes: #1333 Approved by: jlebon --- ...e-repo-static-delta-compilation-analysis.c | 44 +++++++++++++++---- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/src/libostree/ostree-repo-static-delta-compilation-analysis.c b/src/libostree/ostree-repo-static-delta-compilation-analysis.c index f3e69af9..d96604e5 100644 --- a/src/libostree/ostree-repo-static-delta-compilation-analysis.c +++ b/src/libostree/ostree-repo-static-delta-compilation-analysis.c @@ -217,6 +217,37 @@ string_array_nonempty_intersection (GPtrArray *a, return FALSE; } +static gboolean +sizename_is_delta_candidate (OstreeDeltaContentSizeNames *sizename) +{ + /* Don't build candidates for the empty object */ + if (sizename->size == 0) + return FALSE; + + /* Look for known non-delta-able files (currently just compression like xz) */ + for (guint i = 0; i < sizename->basenames->len; i++) + { + const char *name = sizename->basenames->pdata[i]; + /* We could replace this down the line with g_content_type_guess() or so, + * but it's not clear to me that's a major win; we'd still need to + * maintain a list of compression formats, and this doesn't require + * allocation. + * NB: We explicitly don't have .gz here in case someone might be + * using --rsyncable for that. + */ + const char *dot = strrchr (name, '.'); + if (!dot) + continue; + const char *extension = dot+1; + /* Don't add .gz here, see above */ + if (g_str_equal (extension, "xz") || g_str_equal (extension, "bz2")) + return FALSE; + } + + /* Let's try it */ + return TRUE; +} + /* * Build up a map of files with matching basenames and similar size, * and use it to find apparently similar objects. @@ -258,7 +289,7 @@ _ostree_delta_compute_similar_objects (OstreeRepo *repo, &to_sizes, cancellable, error)) goto out; - + /* Iterate over all newly added objects, find objects which have * similar basename and sizes. * @@ -277,8 +308,7 @@ _ostree_delta_compute_similar_objects (OstreeRepo *repo, const guint64 max_threshold = to_sizenames->size * (1.0+similarity_percent_threshold/100.0); - /* Don't build candidates for the empty object */ - if (to_sizenames->size == 0) + if (!sizename_is_delta_candidate (to_sizenames)) continue; for (fuzzy = 0; fuzzy < 2 && !found; fuzzy++) @@ -286,12 +316,8 @@ _ostree_delta_compute_similar_objects (OstreeRepo *repo, for (j = lower; j < upper; j++) { OstreeDeltaContentSizeNames *from_sizenames = from_sizes->pdata[j]; - - /* Don't build candidates for the empty object */ - if (from_sizenames->size == 0) - { - continue; - } + if (!sizename_is_delta_candidate (from_sizenames)) + continue; if (from_sizenames->size < min_threshold) { From 374f7fc9733370f1814e6c97cb03a240bca08814 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 9 Nov 2017 02:15:11 +0000 Subject: [PATCH 16/46] bin/summary: Fix --raw option I wanted to inspect a summary file the other day and was saddened to find it was broken: $ ostree summary --raw error: No option specified; use -u to update summary Fix the test to do the normal thing of passing just --raw without --view. It's legal to pass --raw and --view, but it shouldn't be a requirement. Closes: #1336 Approved by: cgwalters --- src/ostree/ot-builtin-summary.c | 2 +- tests/test-summary-view.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ostree/ot-builtin-summary.c b/src/ostree/ot-builtin-summary.c index c6319f15..ba8f379b 100644 --- a/src/ostree/ot-builtin-summary.c +++ b/src/ostree/ot-builtin-summary.c @@ -210,7 +210,7 @@ ostree_builtin_summary (int argc, char **argv, OstreeCommandInvocation *invocati return FALSE; } } - else if (opt_view) + else if (opt_view || opt_raw) { g_autoptr(GBytes) summary_data = NULL; diff --git a/tests/test-summary-view.sh b/tests/test-summary-view.sh index 52ac8926..6e421079 100755 --- a/tests/test-summary-view.sh +++ b/tests/test-summary-view.sh @@ -55,7 +55,7 @@ assert_file_has_content_literal summary.txt "Timestamp (ostree.commit.timestamp) echo "ok view summary" # Check the summary can be viewed raw too. -${OSTREE} summary --view --raw > raw-summary.txt +${OSTREE} summary --raw > raw-summary.txt assert_file_has_content_literal raw-summary.txt "('main', (" assert_file_has_content_literal raw-summary.txt "('other', (" assert_file_has_content_literal raw-summary.txt "{'ostree.summary.last-modified': Date: Thu, 9 Nov 2017 13:34:16 +0000 Subject: [PATCH 17/46] lib/remote: Export ostree_remote_get_type symbol Without this, you can't really use OstreeRemote as a GObject, which is a requirement for bindings. This was found when attempting to include OstreeRemote in the GIR, and g-ir-scanner wasn't able to link it's temporary object due to an "undefined reference to `ostree_remote_get_type'" error. Closes: #1337 Approved by: pwithnall --- apidoc/ostree-experimental-sections.txt | 2 ++ src/libostree/libostree-experimental.sym | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt index 309d07fb..fc383922 100644 --- a/apidoc/ostree-experimental-sections.txt +++ b/apidoc/ostree-experimental-sections.txt @@ -19,6 +19,8 @@ OstreeRemote ostree_remote_ref ostree_remote_unref ostree_remote_get_name + +ostree_remote_get_type
diff --git a/src/libostree/libostree-experimental.sym b/src/libostree/libostree-experimental.sym index cbe373cf..3b991c42 100644 --- a/src/libostree/libostree-experimental.sym +++ b/src/libostree/libostree-experimental.sym @@ -89,3 +89,8 @@ global: ostree_repo_finder_override_get_type; ostree_repo_finder_override_new; } LIBOSTREE_2017.12_EXPERIMENTAL; + +LIBOSTREE_2017.14_EXPERIMENTAL { +global: + ostree_remote_get_type; +} LIBOSTREE_2017.13_EXPERIMENTAL; From ed242cdd3b4ca22c531e9fad6c9144ba50b925b1 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 9 Nov 2017 05:47:15 -0800 Subject: [PATCH 18/46] lib: Include OstreeRemote and OstreeCollectionRef in GIR Now that g-ir-scanner is being told about ENABLE_EXPERIMENTAL_API, it can include these types correctly. Drop the __GI_SCANNER__ guards in the header files so that all the declarations are found. After this, you can actually construct the types normally: >>> OSTree.CollectionRef.new('com.example.Foo', 'bar') Closes: #1337 Approved by: pwithnall --- src/libostree/ostree-ref.h | 2 -- src/libostree/ostree-remote.h | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/libostree/ostree-ref.h b/src/libostree/ostree-ref.h index 3cf63ed4..e91fe854 100644 --- a/src/libostree/ostree-ref.h +++ b/src/libostree/ostree-ref.h @@ -48,7 +48,6 @@ typedef struct gchar *ref_name; /* (not nullable) */ } OstreeCollectionRef; -#ifndef __GI_SCANNER__ _OSTREE_PUBLIC GType ostree_collection_ref_get_type (void); @@ -59,7 +58,6 @@ _OSTREE_PUBLIC OstreeCollectionRef *ostree_collection_ref_dup (const OstreeCollectionRef *ref); _OSTREE_PUBLIC void ostree_collection_ref_free (OstreeCollectionRef *ref); -#endif _OSTREE_PUBLIC guint ostree_collection_ref_hash (gconstpointer ref); diff --git a/src/libostree/ostree-remote.h b/src/libostree/ostree-remote.h index 23ee21bc..322fb96e 100644 --- a/src/libostree/ostree-remote.h +++ b/src/libostree/ostree-remote.h @@ -47,14 +47,12 @@ G_BEGIN_DECLS typedef struct OstreeRemote OstreeRemote; #endif -#ifndef __GI_SCANNER__ _OSTREE_PUBLIC GType ostree_remote_get_type (void) G_GNUC_CONST; _OSTREE_PUBLIC OstreeRemote *ostree_remote_ref (OstreeRemote *remote); _OSTREE_PUBLIC void ostree_remote_unref (OstreeRemote *remote); -#endif /* GI_SCANNER */ _OSTREE_PUBLIC const gchar *ostree_remote_get_name (OstreeRemote *remote); From 3e8b7e29fa40b4f5151a79701cb994b0c254654c Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 9 Nov 2017 14:43:36 +0000 Subject: [PATCH 19/46] Revert "lib/pull: Skip ostree_repo_resolve_keyring_for_collection for bindings" This reverts commit 519b30b7e1979fea827ea4fe9b0e9ac4db99d631. Now that the experimental GIR is being built correctly and OstreeRemote is a real boxed type, this can be exposed again. Closes: #1337 Approved by: pwithnall --- src/libostree/ostree-repo-pull.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libostree/ostree-repo-pull.c b/src/libostree/ostree-repo-pull.c index b4310027..c6fe7625 100644 --- a/src/libostree/ostree-repo-pull.c +++ b/src/libostree/ostree-repo-pull.c @@ -5612,11 +5612,8 @@ check_remote_matches_collection_id (OstreeRepo *repo, return g_str_equal (remote_collection_id, collection_id); } -/* FIXME: Export this to bindings once OstreeRemote is properly registered as - * a boxed type. - */ /** - * ostree_repo_resolve_keyring_for_collection: (skip) + * ostree_repo_resolve_keyring_for_collection: * @self: an #OstreeRepo * @collection_id: the collection ID to look up a keyring for * @cancellable: (nullable): a #GCancellable, or %NULL From 6b9ce9d35d9a8d5596b96f72da8f0f3faed3f62c Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 9 Nov 2017 15:54:30 -0500 Subject: [PATCH 20/46] lib/traverse: Port to new style Not prep for anything, was just reading this code a bit while working on rpm-ostree jigdo. Closes: #1338 Approved by: jlebon --- src/libostree/ostree-repo-traverse.c | 94 +++++++++++----------------- 1 file changed, 36 insertions(+), 58 deletions(-) diff --git a/src/libostree/ostree-repo-traverse.c b/src/libostree/ostree-repo-traverse.c index 0d9ddbd9..46c3e547 100644 --- a/src/libostree/ostree-repo-traverse.c +++ b/src/libostree/ostree-repo-traverse.c @@ -56,10 +56,6 @@ ostree_repo_commit_traverse_iter_init_commit (OstreeRepoCommitTraverseIter *it { struct _OstreeRepoRealCommitTraverseIter *real = (struct _OstreeRepoRealCommitTraverseIter*)iter; - gboolean ret = FALSE; - const guchar *csum; - g_autoptr(GVariant) meta_csum_bytes = NULL; - g_autoptr(GVariant) content_csum_bytes = NULL; memset (real, 0, sizeof (*real)); real->initialized = TRUE; @@ -68,21 +64,21 @@ ostree_repo_commit_traverse_iter_init_commit (OstreeRepoCommitTraverseIter *it real->current_dir = NULL; real->idx = 0; + g_autoptr(GVariant) content_csum_bytes = NULL; g_variant_get_child (commit, 6, "@ay", &content_csum_bytes); - csum = ostree_checksum_bytes_peek_validate (content_csum_bytes, error); + const guchar *csum = ostree_checksum_bytes_peek_validate (content_csum_bytes, error); if (!csum) - goto out; + return FALSE; ostree_checksum_inplace_from_bytes (csum, real->checksum_content); + g_autoptr(GVariant) meta_csum_bytes = NULL; g_variant_get_child (commit, 7, "@ay", &meta_csum_bytes); csum = ostree_checksum_bytes_peek_validate (meta_csum_bytes, error); if (!csum) - goto out; + return FALSE; ostree_checksum_inplace_from_bytes (csum, real->checksum_meta); - ret = TRUE; - out: - return ret; + return TRUE; } /** @@ -312,8 +308,6 @@ traverse_iter (OstreeRepo *repo, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - while (TRUE) { g_autoptr(GVariant) key = NULL; @@ -330,12 +324,11 @@ traverse_iter (OstreeRepo *repo, g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) { g_debug ("Ignoring not-found dirmeta"); - ret = TRUE; + return TRUE; /* Note early return */ } - else - g_propagate_error (error, g_steal_pointer (&local_error)); - goto out; + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; } else if (iterres == OSTREE_REPO_COMMIT_ITER_RESULT_END) break; @@ -371,16 +364,14 @@ traverse_iter (OstreeRepo *repo, if (!traverse_dirtree (repo, content_checksum, inout_reachable, ignore_missing_dirs, cancellable, error)) - goto out; + return FALSE; } } else g_assert_not_reached (); } - ret = TRUE; - out: - return ret; + return TRUE; } static gboolean @@ -391,12 +382,9 @@ traverse_dirtree (OstreeRepo *repo, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - g_autoptr(GVariant) dirtree = NULL; - ostree_cleanup_repo_commit_traverse_iter - OstreeRepoCommitTraverseIter iter = { 0, }; g_autoptr(GError) local_error = NULL; + g_autoptr(GVariant) dirtree = NULL; if (!ostree_repo_load_variant (repo, OSTREE_OBJECT_TYPE_DIR_TREE, checksum, &dirtree, &local_error)) { @@ -404,26 +392,25 @@ traverse_dirtree (OstreeRepo *repo, g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) { g_debug ("Ignoring not-found dirmeta %s", checksum); - ret = TRUE; + return TRUE; /* Early return */ } - else - g_propagate_error (error, g_steal_pointer (&local_error)); - goto out; + g_propagate_error (error, g_steal_pointer (&local_error)); + return FALSE; } g_debug ("Traversing dirtree %s", checksum); + ostree_cleanup_repo_commit_traverse_iter + OstreeRepoCommitTraverseIter iter = { 0, }; if (!ostree_repo_commit_traverse_iter_init_dirtree (&iter, repo, dirtree, OSTREE_REPO_COMMIT_TRAVERSE_FLAG_NONE, error)) - goto out; + return FALSE; if (!traverse_iter (repo, &iter, inout_reachable, ignore_missing_dirs, cancellable, error)) - goto out; + return FALSE; - ret = TRUE; - out: - return ret; + return TRUE; } /** @@ -446,28 +433,21 @@ ostree_repo_traverse_commit_union (OstreeRepo *repo, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; g_autofree char *tmp_checksum = NULL; while (TRUE) { - gboolean recurse = FALSE; - g_autoptr(GVariant) key = NULL; - g_autoptr(GVariant) commit = NULL; - ostree_cleanup_repo_commit_traverse_iter - OstreeRepoCommitTraverseIter iter = { 0, }; - OstreeRepoCommitState commitstate; - gboolean ignore_missing_dirs = FALSE; - - key = g_variant_ref_sink (ostree_object_name_serialize (commit_checksum, OSTREE_OBJECT_TYPE_COMMIT)); + g_autoptr(GVariant) key = + g_variant_ref_sink (ostree_object_name_serialize (commit_checksum, OSTREE_OBJECT_TYPE_COMMIT)); if (g_hash_table_contains (inout_reachable, key)) break; + g_autoptr(GVariant) commit = NULL; if (!ostree_repo_load_variant_if_exists (repo, OSTREE_OBJECT_TYPE_COMMIT, commit_checksum, &commit, error)) - goto out; + return FALSE; /* Just return if the parent isn't found; we do expect most * people to have partial repositories. @@ -476,10 +456,12 @@ ostree_repo_traverse_commit_union (OstreeRepo *repo, break; /* See if the commit is partial, if so it's not an error to lack objects */ + OstreeRepoCommitState commitstate; if (!ostree_repo_load_commit (repo, commit_checksum, NULL, &commitstate, error)) - goto out; + return FALSE; + gboolean ignore_missing_dirs = FALSE; if ((commitstate & OSTREE_REPO_COMMIT_STATE_PARTIAL) != 0) ignore_missing_dirs = TRUE; @@ -487,14 +469,17 @@ ostree_repo_traverse_commit_union (OstreeRepo *repo, key = NULL; g_debug ("Traversing commit %s", commit_checksum); + ostree_cleanup_repo_commit_traverse_iter + OstreeRepoCommitTraverseIter iter = { 0, }; if (!ostree_repo_commit_traverse_iter_init_commit (&iter, repo, commit, OSTREE_REPO_COMMIT_TRAVERSE_FLAG_NONE, error)) - goto out; + return FALSE; if (!traverse_iter (repo, &iter, inout_reachable, ignore_missing_dirs, cancellable, error)) - goto out; + return FALSE; + gboolean recurse = FALSE; if (maxdepth == -1 || maxdepth > 0) { g_free (tmp_checksum); @@ -511,9 +496,7 @@ ostree_repo_traverse_commit_union (OstreeRepo *repo, break; } - ret = TRUE; - out: - return ret; + return TRUE; } /** @@ -536,17 +519,12 @@ ostree_repo_traverse_commit (OstreeRepo *repo, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - g_autoptr(GHashTable) ret_reachable = - ostree_repo_traverse_new_reachable (); - + g_autoptr(GHashTable) ret_reachable = ostree_repo_traverse_new_reachable (); if (!ostree_repo_traverse_commit_union (repo, commit_checksum, maxdepth, ret_reachable, cancellable, error)) - goto out; + return FALSE; - ret = TRUE; if (out_reachable) *out_reachable = g_steal_pointer (&ret_reachable); - out: - return ret; + return TRUE; } From f7568dbfc8b9a38ad5b81179c83ae5a929b7c9a1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 14 Nov 2017 16:13:13 +0000 Subject: [PATCH 21/46] lib/repo: Add (transfer) annotations to various GHashTable arguments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By default, unless it’s const, an (out) GHashTable will be assumed to be (transfer full). That means the binding needs to free all the items in the hash table, plus the table itself. However, all the GHashTables we use have free functions set already, so freeing the hash table will free its items. This results in a double-free. Fix that by ensuring we annotate such (out) hash tables as (transfer container). Also annotate some other hash tables as (transfer none) where appropriate, for clarity. This fixes OSTree.Repo.list_collection_refs() in the Python bindings. Signed-off-by: Philip Withnall Closes: #1341 Approved by: dbnicholson --- src/libostree/ostree-repo-finder.c | 4 ++-- src/libostree/ostree-repo-refs.c | 15 ++++++++++----- src/libostree/ostree-soup-form.c | 2 +- src/libostree/ostree-soup-uri.c | 2 +- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/libostree/ostree-repo-finder.c b/src/libostree/ostree-repo-finder.c index 576a690c..9be176af 100644 --- a/src/libostree/ostree-repo-finder.c +++ b/src/libostree/ostree-repo-finder.c @@ -432,8 +432,8 @@ G_DEFINE_BOXED_TYPE (OstreeRepoFinderResult, ostree_repo_finder_result, * result * @priority: static priority of the result, where higher numbers indicate lower * priority - * @ref_to_checksum: (element-type OstreeCollectionRef utf8): map of collection–ref pairs - * to checksums provided by this result + * @ref_to_checksum: (element-type OstreeCollectionRef utf8) (transfer none): + * map of collection–ref pairs to checksums provided by this result * @summary_last_modified: Unix timestamp (seconds since the epoch, UTC) when * the summary file for the result was last modified, or `0` if this is unknown * diff --git a/src/libostree/ostree-repo-refs.c b/src/libostree/ostree-repo-refs.c index 9289bb37..ed496253 100644 --- a/src/libostree/ostree-repo-refs.c +++ b/src/libostree/ostree-repo-refs.c @@ -725,7 +725,8 @@ _ostree_repo_list_refs_internal (OstreeRepo *self, * ostree_repo_list_refs: * @self: Repo * @refspec_prefix: (allow-none): Only list refs which match this prefix - * @out_all_refs: (out) (element-type utf8 utf8): Mapping from ref to checksum + * @out_all_refs: (out) (element-type utf8 utf8) (transfer container): + * Mapping from ref to checksum * @cancellable: Cancellable * @error: Error * @@ -750,7 +751,8 @@ ostree_repo_list_refs (OstreeRepo *self, * ostree_repo_list_refs_ext: * @self: Repo * @refspec_prefix: (allow-none): Only list refs which match this prefix - * @out_all_refs: (out) (element-type utf8 utf8): Mapping from ref to checksum + * @out_all_refs: (out) (element-type utf8 utf8) (transfer container): + * Mapping from ref to checksum * @flags: Options controlling listing behavior * @cancellable: Cancellable * @error: Error @@ -778,7 +780,8 @@ ostree_repo_list_refs_ext (OstreeRepo *self, * ostree_repo_remote_list_refs: * @self: Repo * @remote_name: Name of the remote. - * @out_all_refs: (out) (element-type utf8 utf8): Mapping from ref to checksum + * @out_all_refs: (out) (element-type utf8 utf8) (transfer container): + * Mapping from ref to checksum * @cancellable: Cancellable * @error: Error * @@ -893,7 +896,8 @@ remote_list_collection_refs_process_refs (OstreeRepo *self, * ostree_repo_remote_list_collection_refs: * @self: Repo * @remote_name: Name of the remote. - * @out_all_refs: (out) (element-type OstreeCollectionRef utf8): Mapping from collection–ref to checksum + * @out_all_refs: (out) (element-type OstreeCollectionRef utf8) (transfer container): + * Mapping from collection–ref to checksum * @cancellable: Cancellable * @error: Error * @@ -1165,7 +1169,8 @@ _ostree_repo_update_collection_refs (OstreeRepo *self, * ostree_repo_list_collection_refs: * @self: Repo * @match_collection_id: (nullable): If non-%NULL, only list refs from this collection - * @out_all_refs: (out) (element-type OstreeCollectionRef utf8): Mapping from collection–ref to checksum + * @out_all_refs: (out) (element-type OstreeCollectionRef utf8) (transfer container): + * Mapping from collection–ref to checksum * @flags: Options controlling listing behavior * @cancellable: Cancellable * @error: Error diff --git a/src/libostree/ostree-soup-form.c b/src/libostree/ostree-soup-form.c index 74f9c7bb..dfaffb96 100644 --- a/src/libostree/ostree-soup-form.c +++ b/src/libostree/ostree-soup-form.c @@ -82,7 +82,7 @@ encode_pair (GString *str, const char *name, const char *value) /** * soup_form_encode_hash: - * @form_data_set: (element-type utf8 utf8): a hash table containing + * @form_data_set: (element-type utf8 utf8) (transfer none): a hash table containing * name/value pairs (as strings) * * Encodes @form_data_set into a value of type diff --git a/src/libostree/ostree-soup-uri.c b/src/libostree/ostree-soup-uri.c index 97f74636..a3fa2acc 100644 --- a/src/libostree/ostree-soup-uri.c +++ b/src/libostree/ostree-soup-uri.c @@ -1281,7 +1281,7 @@ soup_uri_set_query (SoupURI *uri, const char *query) /** * soup_uri_set_query_from_form: * @uri: a #SoupURI - * @form: (element-type utf8 utf8): a #GHashTable containing HTML form + * @form: (element-type utf8 utf8) (transfer none): a #GHashTable containing HTML form * information * * Sets @uri's query to the result of encoding @form according to the From 4a58364cfaccc231f7f1908f04f9e92ebf9307a1 Mon Sep 17 00:00:00 2001 From: Philip Withnall Date: Tue, 14 Nov 2017 16:15:34 +0000 Subject: [PATCH 22/46] lib/repo: Fix a memory leak of options in ostree_repo_create() Signed-off-by: Philip Withnall Closes: #1341 Approved by: dbnicholson --- src/libostree/ostree-repo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 7f2929b7..22214056 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -1918,8 +1918,9 @@ ostree_repo_create (OstreeRepo *self, g_variant_new_variant (g_variant_new_string (self->collection_id))); glnx_autofd int repo_dir_fd = -1; + g_autoptr(GVariant) options = g_variant_ref_sink (g_variant_builder_end (builder)); if (!repo_create_at_internal (AT_FDCWD, repopath, mode, - g_variant_builder_end (builder), + options, &repo_dir_fd, cancellable, error)) return FALSE; From 20996d0da30377e029495c88120d2dc014976685 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Lopez Perez Date: Mon, 6 Nov 2017 17:29:13 +0100 Subject: [PATCH 23/46] grub-generator: If OSTREE_BOOT_PARTITION is not set, default to /boot Closes: #1326 Approved by: cgwalters --- src/boot/grub2/ostree-grub-generator | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/boot/grub2/ostree-grub-generator b/src/boot/grub2/ostree-grub-generator index 5673b264..82e66bd7 100644 --- a/src/boot/grub2/ostree-grub-generator +++ b/src/boot/grub2/ostree-grub-generator @@ -61,7 +61,12 @@ read_config() populate_menu() { - boot_prefix="${OSTREE_BOOT_PARTITION}" + # Default to /boot if OSTREE_BOOT_PARTITION is not set and /boot is on the same device than ostree/repo + if [ -z ${OSTREE_BOOT_PARTITION+x} ] && [ -d /boot/ostree ] && [ -d /ostree/repo ] && [ $(stat -c '%d' /boot/ostree) -eq $(stat -c '%d' /ostree/repo) ]; then + boot_prefix="/boot" + else + boot_prefix="${OSTREE_BOOT_PARTITION}" + fi for config in $(ls ${entries_path}); do read_config ${config} menu="${menu}menuentry '${title}' {\n" From 23db56f9c36f4e2b16cc42f97fd9c7147bcf0d3d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 15 Nov 2017 18:04:31 -0500 Subject: [PATCH 24/46] bin: Port a few commands (diff,remote,static-delta) to new style No functional changes, not prep for anything, just keeping up some momentum. Closes: #1344 Approved by: jlebon --- src/ostree/ot-admin-builtin-diff.c | 43 ++++-------- src/ostree/ot-builtin-remote.c | 19 ++--- src/ostree/ot-builtin-static-delta.c | 100 +++++++++++---------------- 3 files changed, 60 insertions(+), 102 deletions(-) diff --git a/src/ostree/ot-admin-builtin-diff.c b/src/ostree/ot-admin-builtin-diff.c index fe0c5365..d33fd8a2 100644 --- a/src/ostree/ot-admin-builtin-diff.c +++ b/src/ostree/ot-admin-builtin-diff.c @@ -43,29 +43,20 @@ static GOptionEntry options[] = { gboolean ot_admin_builtin_diff (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) { - g_autoptr(GOptionContext) context = NULL; - g_autoptr(OstreeSysroot) sysroot = NULL; - gboolean ret = FALSE; - g_autoptr(OstreeDeployment) deployment = NULL; - g_autoptr(GFile) deployment_dir = NULL; - g_autoptr(GPtrArray) modified = NULL; - g_autoptr(GPtrArray) removed = NULL; - g_autoptr(GPtrArray) added = NULL; - g_autoptr(GFile) orig_etc_path = NULL; - g_autoptr(GFile) new_etc_path = NULL; - - context = g_option_context_new (""); - + g_autoptr(GOptionContext) context = g_option_context_new (""); g_option_context_add_main_entries (context, options, NULL); + g_autoptr(OstreeSysroot) sysroot = NULL; if (!ostree_admin_option_context_parse (context, options, &argc, &argv, OSTREE_ADMIN_BUILTIN_FLAG_SUPERUSER | OSTREE_ADMIN_BUILTIN_FLAG_UNLOCKED, invocation, &sysroot, cancellable, error)) - goto out; + return FALSE; if (!ot_admin_require_booted_deployment_or_osname (sysroot, opt_osname, cancellable, error)) - goto out; + return FALSE; + + g_autoptr(OstreeDeployment) deployment = NULL; if (opt_osname != NULL) { deployment = ostree_sysroot_get_merge_deployment (sysroot, opt_osname); @@ -73,28 +64,24 @@ ot_admin_builtin_diff (int argc, char **argv, OstreeCommandInvocation *invocatio { g_set_error (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND, "No deployment for OS '%s'", opt_osname); - goto out; + return FALSE; } } else deployment = g_object_ref (ostree_sysroot_get_booted_deployment (sysroot)); - deployment_dir = ostree_sysroot_get_deployment_directory (sysroot, deployment); - - orig_etc_path = g_file_resolve_relative_path (deployment_dir, "usr/etc"); - new_etc_path = g_file_resolve_relative_path (deployment_dir, "etc"); - - modified = g_ptr_array_new_with_free_func ((GDestroyNotify) ostree_diff_item_unref); - removed = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); - added = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); + g_autoptr(GFile) deployment_dir = ostree_sysroot_get_deployment_directory (sysroot, deployment); + g_autoptr(GFile) orig_etc_path = g_file_resolve_relative_path (deployment_dir, "usr/etc"); + g_autoptr(GFile) new_etc_path = g_file_resolve_relative_path (deployment_dir, "etc"); + g_autoptr(GPtrArray) modified = g_ptr_array_new_with_free_func ((GDestroyNotify) ostree_diff_item_unref); + g_autoptr(GPtrArray) removed = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); + g_autoptr(GPtrArray) added = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); if (!ostree_diff_dirs (OSTREE_DIFF_FLAGS_IGNORE_XATTRS, orig_etc_path, new_etc_path, modified, removed, added, cancellable, error)) - goto out; + return FALSE; ostree_diff_print (orig_etc_path, new_etc_path, modified, removed, added); - ret = TRUE; - out: - return ret; + return TRUE; } diff --git a/src/ostree/ot-builtin-remote.c b/src/ostree/ot-builtin-remote.c index 0be878cc..dfb07d03 100644 --- a/src/ostree/ot-builtin-remote.c +++ b/src/ostree/ot-builtin-remote.c @@ -86,12 +86,8 @@ remote_option_context_new_with_commands (void) gboolean ostree_builtin_remote (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) { - OstreeCommand *subcommand; const char *subcommand_name = NULL; - g_autofree char *prgname = NULL; - gboolean ret = FALSE; - int in, out; - + int in,out; for (in = 1, out = 1; in < argc; in++, out++) { /* The non-option is the command, take it out of the arguments */ @@ -115,7 +111,7 @@ ostree_builtin_remote (int argc, char **argv, OstreeCommandInvocation *invocatio argc = out; - subcommand = remote_subcommands; + OstreeCommand *subcommand = remote_subcommands; while (subcommand->name) { if (g_strcmp0 (subcommand_name, subcommand->name) == 0) @@ -150,18 +146,15 @@ ostree_builtin_remote (int argc, char **argv, OstreeCommandInvocation *invocatio help = g_option_context_get_help (context, FALSE, NULL); g_printerr ("%s", help); - goto out; + return FALSE; } - prgname = g_strdup_printf ("%s %s", g_get_prgname (), subcommand_name); + g_autofree char *prgname = g_strdup_printf ("%s %s", g_get_prgname (), subcommand_name); g_set_prgname (prgname); OstreeCommandInvocation sub_invocation = { .command = subcommand }; if (!subcommand->fn (argc, argv, &sub_invocation, cancellable, error)) - goto out; + return FALSE; - ret = TRUE; - - out: - return ret; + return TRUE; } diff --git a/src/ostree/ot-builtin-static-delta.c b/src/ostree/ot-builtin-static-delta.c index d053500a..d91acd20 100644 --- a/src/ostree/ot-builtin-static-delta.c +++ b/src/ostree/ot-builtin-static-delta.c @@ -149,77 +149,63 @@ ot_static_delta_builtin_list (int argc, char **argv, OstreeCommandInvocation *in static gboolean ot_static_delta_builtin_show (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - g_autoptr(GOptionContext) context = NULL; + + g_autoptr(GOptionContext) context = g_option_context_new (""); + g_autoptr(OstreeRepo) repo = NULL; - const char *delta_id = NULL; - - context = g_option_context_new (""); - if (!ostree_option_context_parse (context, list_options, &argc, &argv, invocation, &repo, cancellable, error)) - goto out; + return FALSE; if (argc < 3) { g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "DELTA must be specified"); - goto out; + return FALSE; } - delta_id = argv[2]; + const char *delta_id = argv[2]; if (!ostree_cmd__private__ ()->ostree_static_delta_dump (repo, delta_id, cancellable, error)) - goto out; - - ret = TRUE; - out: - return ret; + return FALSE; + + return TRUE; } static gboolean ot_static_delta_builtin_delete (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - g_autoptr(GOptionContext) context = NULL; + g_autoptr(GOptionContext) context = g_option_context_new (""); + g_autoptr(OstreeRepo) repo = NULL; - const char *delta_id = NULL; - - context = g_option_context_new (""); - if (!ostree_option_context_parse (context, list_options, &argc, &argv, invocation, &repo, cancellable, error)) - goto out; + return FALSE; if (argc < 3) { g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "DELTA must be specified"); - goto out; + return FALSE; } - delta_id = argv[2]; + const char *delta_id = argv[2]; if (!ostree_cmd__private__ ()->ostree_static_delta_delete (repo, delta_id, cancellable, error)) - goto out; + return FALSE; - ret = TRUE; - out: - return ret; + return TRUE; } static gboolean ot_static_delta_builtin_generate (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - g_autoptr(GOptionContext) context = NULL; + g_autoptr(GOptionContext) context = g_option_context_new ("[TO]"); g_autoptr(OstreeRepo) repo = NULL; - - context = g_option_context_new ("[TO]"); if (!ostree_option_context_parse (context, generate_options, &argc, &argv, invocation, &repo, cancellable, error)) - goto out; + return FALSE; if (!ostree_ensure_repo_writable (repo, error)) - goto out; + return FALSE; if (argc >= 3 && opt_to_rev == NULL) opt_to_rev = argv[2]; @@ -228,7 +214,7 @@ ot_static_delta_builtin_generate (int argc, char **argv, OstreeCommandInvocation { g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "TO revision must be specified"); - goto out; + return FALSE; } else { @@ -247,7 +233,7 @@ ot_static_delta_builtin_generate (int argc, char **argv, OstreeCommandInvocation { g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Cannot specify both --empty and --from=REV"); - goto out; + return FALSE; } from_source = NULL; } @@ -264,25 +250,24 @@ ot_static_delta_builtin_generate (int argc, char **argv, OstreeCommandInvocation if (from_source) { if (!ostree_repo_resolve_rev (repo, from_source, FALSE, &from_resolved, error)) - goto out; + return FALSE; } if (!ostree_repo_resolve_rev (repo, opt_to_rev, FALSE, &to_resolved, error)) - goto out; + return FALSE; if (opt_if_not_exists) { gboolean does_exist; g_autofree char *delta_id = from_resolved ? g_strconcat (from_resolved, "-", to_resolved, NULL) : g_strdup (to_resolved); if (!ostree_cmd__private__ ()->ostree_static_delta_query_exists (repo, delta_id, &does_exist, cancellable, error)) - goto out; + return FALSE; if (does_exist) { g_print ("Delta %s already exists.\n", delta_id); - ret = TRUE; - goto out; + return TRUE; } } - + if (opt_endianness) { if (strcmp (opt_endianness, "l") == 0) @@ -293,12 +278,12 @@ ot_static_delta_builtin_generate (int argc, char **argv, OstreeCommandInvocation { g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Invalid endianness '%s'", opt_endianness); - goto out; + return FALSE; } } else endianness = G_BYTE_ORDER; - + if (opt_swap_endianness) { switch (endianness) @@ -346,54 +331,47 @@ ot_static_delta_builtin_generate (int argc, char **argv, OstreeCommandInvocation from_resolved, to_resolved, NULL, params, cancellable, error)) - goto out; + return FALSE; } } - ret = TRUE; - out: - return ret; + return TRUE; } static gboolean ot_static_delta_builtin_apply_offline (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error) { - gboolean ret = FALSE; - const char *patharg; - g_autoptr(GFile) path = NULL; g_autoptr(GOptionContext) context = NULL; g_autoptr(OstreeRepo) repo = NULL; context = g_option_context_new (""); if (!ostree_option_context_parse (context, apply_offline_options, &argc, &argv, invocation, &repo, cancellable, error)) - goto out; + return FALSE; if (!ostree_ensure_repo_writable (repo, error)) - goto out; + return FALSE; if (argc < 3) { g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, "PATH must be specified"); - goto out; + return FALSE; } - patharg = argv[2]; - path = g_file_new_for_path (patharg); + const char *patharg = argv[2]; + g_autoptr(GFile) path = g_file_new_for_path (patharg); if (!ostree_repo_prepare_transaction (repo, NULL, cancellable, error)) - goto out; + return FALSE; if (!ostree_repo_static_delta_execute_offline (repo, path, FALSE, cancellable, error)) - goto out; + return FALSE; if (!ostree_repo_commit_transaction (repo, NULL, cancellable, error)) - goto out; + return FALSE; - ret = TRUE; - out: - return ret; + return TRUE; } gboolean From d04debb4fb28395e6567805bc16a10e6a3af10a2 Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Thu, 16 Nov 2017 14:20:07 +0000 Subject: [PATCH 25/46] build: fix "executible" typo Closes: #1345 Approved by: jlebon --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 6b095504..a1271909 100644 --- a/configure.ac +++ b/configure.ac @@ -503,7 +503,7 @@ AS_IF([test x$with_grub2_mkconfig_path = x], [ dnl on some 'grub'. We default to grub2-mkconfig. AC_CHECK_PROGS(GRUB2_MKCONFIG, [grub2-mkconfig grub-mkconfig], [grub2-mkconfig]) ],[GRUB2_MKCONFIG=$with_grub2_mkconfig_path]) -AC_DEFINE_UNQUOTED([GRUB2_MKCONFIG_PATH], ["$GRUB2_MKCONFIG"], [The system grub2-mkconfig executible name]) +AC_DEFINE_UNQUOTED([GRUB2_MKCONFIG_PATH], ["$GRUB2_MKCONFIG"], [The system grub2-mkconfig executable name]) AC_ARG_WITH(static-compiler, AS_HELP_STRING([--with-static-compiler], From c60f31962947b5cc83fb939f854903f1ff837cf0 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 17 Nov 2017 16:54:33 +0000 Subject: [PATCH 26/46] lib/repo: Add debug messages when allocating tmpdir This code is pretty complex and has some races when reusing tmpdirs, so print some messages for debugging. Closes: #1346 Approved by: cgwalters --- src/libostree/ostree-repo.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 22214056..82695800 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4979,6 +4979,7 @@ _ostree_repo_allocate_tmpdir (int tmpdir_dfd, (void)futimens (target_dfd, NULL); /* We found an existing tmpdir which we managed to lock */ + g_debug ("Reusing tmpdir %s", dent->d_name); ret_tmpdir.src_dfd = tmpdir_dfd; ret_tmpdir.fd = glnx_steal_fd (&target_dfd); ret_tmpdir.path = g_strdup (dent->d_name); @@ -5003,6 +5004,7 @@ _ostree_repo_allocate_tmpdir (int tmpdir_dfd, if (!did_lock) continue; + g_debug ("Using new tmpdir %s", new_tmpdir.path); ret_tmpdir = new_tmpdir; /* Transfer ownership */ new_tmpdir.initialized = FALSE; } From f246287010015f75b939501935936163340b4cea Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 17 Nov 2017 16:55:43 +0000 Subject: [PATCH 27/46] lib/repo: Restore tmpdir reusing out parameter This got lost in d0b0578 and now the caller always thinks it got a new tmpdir. Closes: #1346 Approved by: cgwalters --- src/libostree/ostree-repo.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 82695800..c1998825 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4980,6 +4980,7 @@ _ostree_repo_allocate_tmpdir (int tmpdir_dfd, /* We found an existing tmpdir which we managed to lock */ g_debug ("Reusing tmpdir %s", dent->d_name); + reusing_dir = TRUE; ret_tmpdir.src_dfd = tmpdir_dfd; ret_tmpdir.fd = glnx_steal_fd (&target_dfd); ret_tmpdir.path = g_strdup (dent->d_name); From bf85f8d89e125d6c5f83bc4334533529e4a17c03 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 17 Nov 2017 16:58:00 +0000 Subject: [PATCH 28/46] lib/repo: Handle race with existing tmpdir being deleted Another tmpdir user may have deleted an existing tmpdir between the time the current user called readdir and tried to open it. Closes: #1346 Approved by: cgwalters --- src/libostree/ostree-repo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index c1998825..768c9ce7 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4955,7 +4955,8 @@ _ostree_repo_allocate_tmpdir (int tmpdir_dfd, if (!glnx_opendirat (dfd_iter.fd, dent->d_name, FALSE, &target_dfd, &local_error)) { - if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_DIRECTORY)) + if (g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_DIRECTORY) || + g_error_matches (local_error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) continue; else { From 162edf71ed672f92ae9c63ace6e75b0c94ab0357 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 17 Nov 2017 17:06:32 +0000 Subject: [PATCH 29/46] lib/repo: Don't delete new tmpdir if it can't be locked If a newly allocated tmpdir can't be locked, set initialized to FALSE so that glnx_tmpdir_cleanup doesn't delete it when new_tmpdir goes out of scope. Closes: #1346 Approved by: cgwalters --- src/libostree/ostree-repo.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 768c9ce7..6e342125 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -5004,7 +5004,16 @@ _ostree_repo_allocate_tmpdir (int tmpdir_dfd, error)) return FALSE; if (!did_lock) - continue; + { + /* We raced and someone else already locked the newly created + * directory. Free the resources here and then mark it as + * uninitialized so glnx_tmpdir_cleanup doesn't delete the directory + * when new_tmpdir goes out of scope. + */ + glnx_tmpdir_unset (&new_tmpdir); + new_tmpdir.initialized = FALSE; + continue; + } g_debug ("Using new tmpdir %s", new_tmpdir.path); ret_tmpdir = new_tmpdir; /* Transfer ownership */ From 682e5277f09a66f217c0834f22a03b561e2f37a9 Mon Sep 17 00:00:00 2001 From: Dusty Mabe Date: Sun, 26 Nov 2017 11:14:11 -0500 Subject: [PATCH 30/46] add back helpful --allow-downgrade err message Closes: #1348 Approved by: cgwalters --- src/libostree/ostree-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c index ea4c6cc3..c029aa47 100644 --- a/src/libostree/ostree-core.c +++ b/src/libostree/ostree-core.c @@ -2204,7 +2204,7 @@ _ostree_compare_timestamps (const char *current_rev, g_autofree char *current_ts_str = g_date_time_format (current_dt, "%c"); g_autofree char *new_ts_str = g_date_time_format (new_dt, "%c"); - return glnx_throw (error, "Upgrade target revision '%s' with timestamp '%s' is chronologically older than current revision '%s' with timestamp '%s'", + return glnx_throw (error, "Upgrade target revision '%s' with timestamp '%s' is chronologically older than current revision '%s' with timestamp '%s'; use --allow-downgrade to permit", new_rev, new_ts_str, current_rev, current_ts_str); } From 33ded5031cc5b211dc0654c6301dfb545267bf7b Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 27 Nov 2017 17:18:19 +0000 Subject: [PATCH 31/46] papr: Bump primary to f27 Let's start with just f27-primary for now. Closes: #1350 Approved by: cgwalters --- .papr.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.papr.yml b/.papr.yml index 04190188..2016c61b 100644 --- a/.papr.yml +++ b/.papr.yml @@ -4,10 +4,10 @@ branches: - try required: true -context: f26-primary +context: f27-primary container: - image: registry.fedoraproject.org/fedora:26 + image: registry.fedoraproject.org/fedora:27 env: # Enable all the sanitizers for this primary build. From 6cbc4a69051f651ec2fe9c0e6d9a7b3dd8906a2d Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 27 Nov 2017 17:18:56 +0000 Subject: [PATCH 32/46] ci: Make sure we save gtdr test results on failures If a test fails, we immediately exit and thus never get a chance to actually upload the test results. Add a trap so that they always uploaded, even on failure. Closes: #1350 Approved by: cgwalters --- ci/build-check.sh | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/ci/build-check.sh b/ci/build-check.sh index 2f4ddaba..6e8b3930 100755 --- a/ci/build-check.sh +++ b/ci/build-check.sh @@ -16,6 +16,17 @@ for x in test-suite.log config.log; do done # And now run the installed tests make install + +copy_out_gdtr_artifacts() { + # Keep this in sync with papr.yml + # TODO; Split the main/clang builds into separate build dirs + for x in test-suite.log config.log gdtr-results; do + if test -e ${resultsdir}/${x}; then + mv ${resultsdir}/${x} ${topdir} + fi + done +} + if test -x /usr/bin/gnome-desktop-testing-runner; then mkdir ${resultsdir}/gdtr-results # Temporary hack @@ -24,6 +35,8 @@ if test -x /usr/bin/gnome-desktop-testing-runner; then env NOCONFIGURE=1 ./autogen.sh ./configure --prefix=/usr --libdir=/usr/lib64 make && rm -f /usr/bin/ginsttest-runner && make install) + # set a trap in case a test fails + trap copy_out_gdtr_artifacts EXIT # Use the new -L option gnome-desktop-testing-runner -L ${resultsdir}/gdtr-results -p 0 ${INSTALLED_TESTS_PATTERN:-libostree/} fi @@ -38,11 +51,3 @@ if test -x /usr/bin/clang; then export CC=clang build fi - -# Keep this in sync with papr.yml -# TODO; Split the main/clang builds into separate build dirs -for x in test-suite.log config.log gdtr-results; do - if test -e ${resultsdir}/${x}; then - mv ${resultsdir}/${x} ${topdir} - fi -done From 82e2150b98bd1ad5149c1e86db22f8ffb2dd6a41 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 27 Nov 2017 11:05:07 -0500 Subject: [PATCH 33/46] fetcher/curl: Stop using CURLOPT_LOW_SPEED_TIME/_LIMIT They don't play nicely currently with HTTP2 where we may have lots of requests queued. https://github.com/ostreedev/ostree/issues/878#issuecomment-347228854 In practice anyways I think issues here are better solved on a higher level - e.g. apps today can use an overall timeout on pulls and if they exceed the limit set the cancellable. Closes: #1349 Approved by: jlebon --- src/libostree/ostree-fetcher-curl.c | 9 +++++++-- tests/test-pull-mirrorlist.sh | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-fetcher-curl.c b/src/libostree/ostree-fetcher-curl.c index 8a23b163..58835529 100644 --- a/src/libostree/ostree-fetcher-curl.c +++ b/src/libostree/ostree-fetcher-curl.c @@ -788,8 +788,13 @@ initiate_next_curl_request (FetcherRequest *req, curl_easy_setopt (req->easy, CURLOPT_PROGRESSFUNCTION, prog_cb); curl_easy_setopt (req->easy, CURLOPT_FOLLOWLOCATION, 1L); curl_easy_setopt (req->easy, CURLOPT_CONNECTTIMEOUT, 30L); - curl_easy_setopt (req->easy, CURLOPT_LOW_SPEED_LIMIT, 1L); - curl_easy_setopt (req->easy, CURLOPT_LOW_SPEED_TIME, 30L); + /* We used to set CURLOPT_LOW_SPEED_LIMIT and CURLOPT_LOW_SPEED_TIME + * here, but see https://github.com/ostreedev/ostree/issues/878#issuecomment-347228854 + * basically those options don't play well with HTTP2 at the moment + * where we can have lots of outstanding requests. Further, + * we could implement that functionality at a higher level + * more consistently too. + */ /* closure bindings -> task */ curl_easy_setopt (req->easy, CURLOPT_PRIVATE, task); diff --git a/tests/test-pull-mirrorlist.sh b/tests/test-pull-mirrorlist.sh index 22d3950b..93b97eec 100755 --- a/tests/test-pull-mirrorlist.sh +++ b/tests/test-pull-mirrorlist.sh @@ -60,7 +60,7 @@ cat > ${test_tmpdir}/ostree-srv/mirrorlist < Date: Tue, 28 Nov 2017 15:23:39 +0100 Subject: [PATCH 34/46] rofiles-fuse: Fix utime() support We use utimens instead of utime, thus allowing nanosecond timestamps, and also fixes a bug where we used to passed UTIME_OMIT to tv_nsec which made the entire operation a no-op. Closes: #1351 Approved by: cgwalters --- src/rofiles-fuse/main.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/rofiles-fuse/main.c b/src/rofiles-fuse/main.c index 97c91b60..9e04274b 100644 --- a/src/rofiles-fuse/main.c +++ b/src/rofiles-fuse/main.c @@ -284,18 +284,11 @@ callback_truncate (const char *path, off_t size) } static int -callback_utime (const char *path, struct utimbuf *buf) +callback_utimens (const char *path, const struct timespec tv[2]) { - struct timespec ts[2]; - path = ENSURE_RELPATH (path); - ts[0].tv_sec = buf->actime; - ts[0].tv_nsec = UTIME_OMIT; - ts[1].tv_sec = buf->modtime; - ts[1].tv_nsec = UTIME_OMIT; - - if (utimensat (basefd, path, ts, AT_SYMLINK_NOFOLLOW) == -1) + if (utimensat (basefd, path, tv, AT_SYMLINK_NOFOLLOW) == -1) return -errno; return 0; @@ -506,7 +499,7 @@ struct fuse_operations callback_oper = { .chmod = callback_chmod, .chown = callback_chown, .truncate = callback_truncate, - .utime = callback_utime, + .utimens = callback_utimens, .create = callback_create, .open = callback_open, .read_buf = callback_read_buf, From a1745e1a79ea1c41cc62e91b346a2a9961997a76 Mon Sep 17 00:00:00 2001 From: Joaquim Rocha Date: Fri, 24 Nov 2017 14:56:28 +0100 Subject: [PATCH 35/46] lib/remote: Add a method to return the URL When using dynamic remotes (LAN and USB), we cannot use their name with the common remote related ops (ostree_repo_remote_...) because ostree doesn't keep this type of remotes in its internal hash table. Unfortunately this means that we cannot access the URL of those remotes either (in order to e.g. set the right URL for those remotes in Flatpak). Since the URL is actually stored in a key file that belongs to the OstreeRemote, then we can simply allow users access to it through a getter. So this patch adds a method that allows to return the URL directly from the OstreeRemote without having to go through the OstreeRepo. The test-repo-finder-config is also updated by this patch to check if the URL is correct. Closes: #1353 Approved by: cgwalters --- apidoc/ostree-experimental-sections.txt | 1 + src/libostree/libostree-experimental.sym | 1 + src/libostree/ostree-remote.c | 18 ++++++++++++++++++ src/libostree/ostree-remote.h | 3 +++ tests/test-repo-finder-config.c | 2 ++ 5 files changed, 25 insertions(+) diff --git a/apidoc/ostree-experimental-sections.txt b/apidoc/ostree-experimental-sections.txt index fc383922..60daaca5 100644 --- a/apidoc/ostree-experimental-sections.txt +++ b/apidoc/ostree-experimental-sections.txt @@ -19,6 +19,7 @@ OstreeRemote ostree_remote_ref ostree_remote_unref ostree_remote_get_name +ostree_remote_get_url ostree_remote_get_type
diff --git a/src/libostree/libostree-experimental.sym b/src/libostree/libostree-experimental.sym index 3b991c42..b83ad1b0 100644 --- a/src/libostree/libostree-experimental.sym +++ b/src/libostree/libostree-experimental.sym @@ -93,4 +93,5 @@ global: LIBOSTREE_2017.14_EXPERIMENTAL { global: ostree_remote_get_type; + ostree_remote_get_url; } LIBOSTREE_2017.13_EXPERIMENTAL; diff --git a/src/libostree/ostree-remote.c b/src/libostree/ostree-remote.c index 605a7eb9..b75640e7 100644 --- a/src/libostree/ostree-remote.c +++ b/src/libostree/ostree-remote.c @@ -180,3 +180,21 @@ ostree_remote_get_name (OstreeRemote *remote) return remote->name; } + +/** + * ostree_remote_get_url: + * @remote: an #OstreeRemote + * + * Get the URL from the remote. + * + * Returns: (transfer full): the remote's URL + * Since: 2017.14 + */ +gchar * +ostree_remote_get_url (OstreeRemote *remote) +{ + g_return_val_if_fail (remote != NULL, NULL); + g_return_val_if_fail (remote->ref_count > 0, NULL); + + return g_key_file_get_string (remote->options, remote->group, "url", NULL); +} diff --git a/src/libostree/ostree-remote.h b/src/libostree/ostree-remote.h index 322fb96e..aa9bf731 100644 --- a/src/libostree/ostree-remote.h +++ b/src/libostree/ostree-remote.h @@ -57,4 +57,7 @@ void ostree_remote_unref (OstreeRemote *remote); _OSTREE_PUBLIC const gchar *ostree_remote_get_name (OstreeRemote *remote); +_OSTREE_PUBLIC +gchar *ostree_remote_get_url (OstreeRemote *remote); + G_END_DECLS diff --git a/tests/test-repo-finder-config.c b/tests/test-repo-finder-config.c index 428e02eb..7eff53d2 100644 --- a/tests/test-repo-finder-config.c +++ b/tests/test-repo-finder-config.c @@ -284,11 +284,13 @@ test_repo_finder_config_mixed_configs (Fixture *fixture, g_assert_cmpuint (g_hash_table_size (result->ref_to_checksum), ==, 2); g_assert_true (g_hash_table_contains (result->ref_to_checksum, &ref0)); g_assert_true (g_hash_table_contains (result->ref_to_checksum, &ref1)); + g_assert_cmpstr (ostree_remote_get_url (result->remote), ==, collection0_uri); } else if (g_strcmp0 (ostree_remote_get_name (result->remote), "remote1") == 0) { g_assert_cmpuint (g_hash_table_size (result->ref_to_checksum), ==, 1); g_assert_true (g_hash_table_contains (result->ref_to_checksum, &ref3)); + g_assert_cmpstr (ostree_remote_get_url (result->remote), ==, collection1_uri); } else { From bd6a15e7a32b1879c301ac1a77bbe4de1cee523d Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 28 Nov 2017 15:17:10 -0500 Subject: [PATCH 36/46] lib/commit: Use direct repo writes if fsync is disabled For situations where fsync is disabled, there's basically no reason to do the whole "staging directory" dance. Just write directly into the repo. Today I use `fsync=false` for my build/cache repos. I briefly considered not allocating a tmpdir at all in this case, but we actually do want the txn tmpdir for the non-`O_TMPFILE` case. Part of https://github.com/ostreedev/ostree/issues/1184 Closes: #1354 Approved by: giuseppe --- src/libostree/ostree-repo-commit.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index cf1a513f..e43a0fa7 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -38,13 +38,14 @@ #include "ostree-checksum-input-stream.h" #include "ostree-varint.h" -/* In most cases, we write into a staging dir for commit, but we also allow - * direct writes into objects/ for e.g. hardlink imports. +/* If fsync is enabled and we're in a txn, we write into a staging dir for + * commit, but we also allow direct writes into objects/ for e.g. hardlink + * imports. */ static int commit_dest_dfd (OstreeRepo *self) { - if (self->in_transaction) + if (self->in_transaction && !self->disable_fsync) return self->commit_stagedir.fd; else return self->objects_dir_fd; From 17308e21493b4bbe1eb9a7b03cdd90209dfc37a8 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 13 Oct 2017 21:01:20 -0400 Subject: [PATCH 37/46] lib/repo: Add a new private API for bare content writes This lowers into the commit core what the static delta code was doing, and improves the API. The bigger picture issue is that for writing large files, our current "pull" API where the caller provides a `GInputStream` is very awkward in some scenarios. For example, we have a whole "libarchive input stream" that is a ~200 line GObject that boils down to wrapping `archive_read_data()`. This came more to a head when I was working on rpm-ostree jigdo since I had to copy that object. One step we can take after this is to further split `write_content_object()` into a "write symlink or archive object" versus "write bare content object" (it already has a mess of conditionals) and teach the latter case to call this. The eventual goal here is to make this API public. Closes: #1355 Approved by: jlebon --- src/libostree/ostree-repo-commit.c | 128 ++++++++++++++---- src/libostree/ostree-repo-private.h | 43 ++++-- .../ostree-repo-static-delta-processing.c | 119 ++++------------ 3 files changed, 159 insertions(+), 131 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index e43a0fa7..e283702d 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -405,46 +405,116 @@ add_size_index_to_metadata (OstreeRepo *self, return g_variant_ref_sink (g_variant_builder_end (builder)); } +typedef struct { + gboolean initialized; + GLnxTmpfile tmpf; + char *expected_checksum; + OtChecksum checksum; + guint64 content_len; + guint64 bytes_written; + guint uid; + guint gid; + guint mode; + GVariant *xattrs; +} OstreeRealRepoBareContent; +G_STATIC_ASSERT (sizeof (OstreeRepoBareContent) >= sizeof (OstreeRealRepoBareContent)); + /* Create a tmpfile for writing a bare file. Currently just used * by the static delta code, but will likely later be extended * to be used also by the dfd_iter commit path. */ gboolean -_ostree_repo_open_content_bare (OstreeRepo *self, - const char *checksum, - guint64 content_len, - GLnxTmpfile *out_tmpf, - GCancellable *cancellable, - GError **error) +_ostree_repo_bare_content_open (OstreeRepo *self, + const char *expected_checksum, + guint64 content_len, + guint uid, + guint gid, + guint mode, + GVariant *xattrs, + OstreeRepoBareContent *out_regwrite, + GCancellable *cancellable, + GError **error) { - return glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, - out_tmpf, error); + OstreeRealRepoBareContent *real = (OstreeRealRepoBareContent*) out_regwrite; + g_assert (!real->initialized); + real->initialized = TRUE; + g_assert (S_ISREG (mode)); + if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, + &real->tmpf, error)) + return FALSE; + ot_checksum_init (&real->checksum); + real->expected_checksum = g_strdup (expected_checksum); + real->content_len = content_len; + real->bytes_written = 0; + real->uid = uid; + real->gid = gid; + real->mode = mode; + real->xattrs = xattrs ? g_variant_ref (xattrs) : NULL; + + /* Initialize the checksum with the header info */ + g_autoptr(GFileInfo) finfo = _ostree_mode_uidgid_to_gfileinfo (mode, uid, gid); + g_autoptr(GBytes) header = _ostree_file_header_new (finfo, xattrs); + ot_checksum_update_bytes (&real->checksum, header); + + return TRUE; } -/* Used by static deltas, which have a separate "push" flow for - * regfile objects distinct from the "pull" model used by - * write_content_object(). - */ gboolean -_ostree_repo_commit_trusted_content_bare (OstreeRepo *self, - const char *checksum, - GLnxTmpfile *tmpf, - guint32 uid, - guint32 gid, - guint32 mode, - GVariant *xattrs, - GCancellable *cancellable, - GError **error) +_ostree_repo_bare_content_write (OstreeRepo *repo, + OstreeRepoBareContent *barewrite, + const guint8 *buf, + size_t len, + GCancellable *cancellable, + GError **error) { - /* I don't think this is necessary, but a similar check was here previously, - * keeping it for extra redundancy. - */ - if (!tmpf->initialized || tmpf->fd == -1) - return TRUE; + OstreeRealRepoBareContent *real = (OstreeRealRepoBareContent*) barewrite; + g_assert (real->initialized); + ot_checksum_update (&real->checksum, buf, len); + if (glnx_loop_write (real->tmpf.fd, buf, len) < 0) + return glnx_throw_errno_prefix (error, "write"); + return TRUE; +} - return commit_loose_regfile_object (self, checksum, - tmpf, uid, gid, mode, xattrs, - cancellable, error); +gboolean +_ostree_repo_bare_content_commit (OstreeRepo *self, + OstreeRepoBareContent *barewrite, + char *checksum_buf, + size_t buflen, + GCancellable *cancellable, + GError **error) +{ + OstreeRealRepoBareContent *real = (OstreeRealRepoBareContent*) barewrite; + g_assert (real->initialized); + ot_checksum_get_hexdigest (&real->checksum, checksum_buf, buflen); + + if (real->expected_checksum && + !_ostree_compare_object_checksum (OSTREE_OBJECT_TYPE_FILE, + real->expected_checksum, checksum_buf, + error)) + return FALSE; + + if (!commit_loose_regfile_object (self, checksum_buf, + &real->tmpf, real->uid, real->gid, + real->mode, real->xattrs, + cancellable, error)) + return FALSE; + + /* Let's have a guarantee that after commit the object is cleaned up */ + _ostree_repo_bare_content_cleanup (barewrite); + return TRUE; +} + +void +_ostree_repo_bare_content_cleanup (OstreeRepoBareContent *regwrite) +{ + OstreeRealRepoBareContent *real = (OstreeRealRepoBareContent*) regwrite; + if (!real->initialized) + return; + glnx_tmpfile_clear (&real->tmpf); + ot_checksum_clear (&real->checksum); + g_clear_pointer (&real->expected_checksum, (GDestroyNotify)g_free); + g_clear_pointer (&real->xattrs, (GDestroyNotify)g_variant_unref); + real->initialized = FALSE; } /* Allocate an O_TMPFILE, write everything from @input to it, but diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index d8d16e1a..58c104b9 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -365,24 +365,41 @@ _ostree_repo_commit_tmpf_final (OstreeRepo *self, GCancellable *cancellable, GError **error); +typedef struct { + gboolean initialized; + gpointer opaque0[10]; + guint opaque1[10]; +} OstreeRepoBareContent; +void _ostree_repo_bare_content_cleanup (OstreeRepoBareContent *regwrite); +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(OstreeRepoBareContent, _ostree_repo_bare_content_cleanup) + gboolean -_ostree_repo_open_content_bare (OstreeRepo *self, - const char *checksum, - guint64 content_len, - GLnxTmpfile *out_tmpf, +_ostree_repo_bare_content_open (OstreeRepo *self, + const char *checksum, + guint64 content_len, + guint uid, + guint gid, + guint mode, + GVariant *xattrs, + OstreeRepoBareContent *out_regwrite, GCancellable *cancellable, GError **error); gboolean -_ostree_repo_commit_trusted_content_bare (OstreeRepo *self, - const char *checksum, - GLnxTmpfile *tmpf, - guint32 uid, - guint32 gid, - guint32 mode, - GVariant *xattrs, - GCancellable *cancellable, - GError **error); +_ostree_repo_bare_content_write (OstreeRepo *repo, + OstreeRepoBareContent *barewrite, + const guint8 *buf, + size_t len, + GCancellable *cancellable, + GError **error); + +gboolean +_ostree_repo_bare_content_commit (OstreeRepo *self, + OstreeRepoBareContent *barewrite, + char *checksum_buf, + size_t buflen, + GCancellable *cancellable, + GError **error); gboolean _ostree_repo_load_file_bare (OstreeRepo *self, diff --git a/src/libostree/ostree-repo-static-delta-processing.c b/src/libostree/ostree-repo-static-delta-processing.c index 4545b39f..3b9fd49f 100644 --- a/src/libostree/ostree-repo-static-delta-processing.c +++ b/src/libostree/ostree-repo-static-delta-processing.c @@ -55,11 +55,9 @@ typedef struct { GError **async_error; OstreeObjectType output_objtype; - GLnxTmpfile tmpf; guint64 content_size; - GOutputStream *content_out; - OtChecksum content_checksum; char checksum[OSTREE_SHA256_STRING_LEN+1]; + OstreeRepoBareContent content_out; char *read_source_object; int read_source_fd; gboolean have_obj; @@ -278,9 +276,7 @@ _ostree_static_delta_part_execute (OstreeRepo *repo, ret = TRUE; out: - glnx_tmpfile_clear (&state->tmpf); - g_clear_object (&state->content_out); - ot_checksum_clear (&state->content_checksum); + _ostree_repo_bare_content_cleanup (&state->content_out); return ret; } @@ -378,29 +374,6 @@ validate_ofs (StaticDeltaExecutionState *state, return TRUE; } -static gboolean -content_out_write (OstreeRepo *repo, - StaticDeltaExecutionState *state, - const guint8* buf, - gsize len, - GCancellable *cancellable, - GError **error) -{ - gsize bytes_written; - - if (state->content_checksum.initialized) - ot_checksum_update (&state->content_checksum, buf, len); - - /* Ignore bytes_written since we discard partial content */ - if (!g_output_stream_write_all (state->content_out, - buf, len, - &bytes_written, - cancellable, error)) - return FALSE; - - return TRUE; -} - static gboolean do_content_open_generic (OstreeRepo *repo, StaticDeltaExecutionState *state, @@ -485,33 +458,15 @@ dispatch_bspatch (OstreeRepo *repo, &stream) < 0) return FALSE; - if (!content_out_write (repo, state, buf, state->content_size, - cancellable, error)) + if (!_ostree_repo_bare_content_write (repo, &state->content_out, + buf, state->content_size, + cancellable, error)) return FALSE; } return TRUE; } -/* Before, we had a distinction between "trusted" and "untrusted" deltas - * which we've decided wasn't a good idea. Now, we always checksum the content. - * Compare with what ostree_checksum_file_from_input() is doing too. - */ -static gboolean -handle_untrusted_content_checksum (OstreeRepo *repo, - StaticDeltaExecutionState *state, - GCancellable *cancellable, - GError **error) -{ - g_autoptr(GFileInfo) finfo = _ostree_mode_uidgid_to_gfileinfo (state->mode, state->uid, state->gid); - g_autoptr(GBytes) header = _ostree_file_header_new (finfo, state->xattrs); - - ot_checksum_init (&state->content_checksum); - ot_checksum_update_bytes (&state->content_checksum, header); - - return TRUE; -} - static gboolean dispatch_open_splice_and_close (OstreeRepo *repo, StaticDeltaExecutionState *state, @@ -589,20 +544,18 @@ dispatch_open_splice_and_close (OstreeRepo *repo, if (!state->have_obj) { - if (!_ostree_repo_open_content_bare (repo, state->checksum, + if (!_ostree_repo_bare_content_open (repo, state->checksum, state->content_size, - &state->tmpf, + state->uid, state->gid, state->mode, + state->xattrs, + &state->content_out, cancellable, error)) goto out; - state->content_out = g_unix_output_stream_new (state->tmpf.fd, FALSE); - if (!handle_untrusted_content_checksum (repo, state, cancellable, error)) - goto out; - - if (!content_out_write (repo, state, - state->payload_data + content_offset, - state->content_size, - cancellable, error)) + if (!_ostree_repo_bare_content_write (repo, &state->content_out, + state->payload_data + content_offset, + state->content_size, + cancellable, error)) goto out; } } @@ -691,17 +644,15 @@ dispatch_open (OstreeRepo *repo, if (!state->have_obj) { - if (!_ostree_repo_open_content_bare (repo, state->checksum, + if (!_ostree_repo_bare_content_open (repo, state->checksum, state->content_size, - &state->tmpf, + state->uid, state->gid, state->mode, + state->xattrs, + &state->content_out, cancellable, error)) return FALSE; - state->content_out = g_unix_output_stream_new (state->tmpf.fd, FALSE); } - if (!handle_untrusted_content_checksum (repo, state, cancellable, error)) - return FALSE; - return TRUE; } @@ -740,8 +691,9 @@ dispatch_write (OstreeRepo *repo, if (G_UNLIKELY (bytes_read == 0)) return glnx_throw (error, "Unexpected EOF reading object %s", state->read_source_object); - if (!content_out_write (repo, state, (guint8*)buf, bytes_read, - cancellable, error)) + if (!_ostree_repo_bare_content_write (repo, &state->content_out, + (guint8*)buf, bytes_read, + cancellable, error)) return FALSE; content_size -= bytes_read; @@ -753,8 +705,9 @@ dispatch_write (OstreeRepo *repo, if (!validate_ofs (state, content_offset, content_size, error)) return FALSE; - if (!content_out_write (repo, state, state->payload_data + content_offset, content_size, - cancellable, error)) + if (!_ostree_repo_bare_content_write (repo, &state->content_out, + state->payload_data + content_offset, content_size, + cancellable, error)) return FALSE; } } @@ -818,34 +771,22 @@ dispatch_close (OstreeRepo *repo, { GLNX_AUTO_PREFIX_ERROR("opcode close", error); - if (state->content_out) + if (state->content_out.initialized) { - if (!g_output_stream_flush (state->content_out, cancellable, error)) + char actual_checksum[OSTREE_SHA256_STRING_LEN+1]; + if (!_ostree_repo_bare_content_commit (repo, &state->content_out, actual_checksum, + sizeof (actual_checksum), + cancellable, error)) return FALSE; - if (state->content_checksum.initialized) - { - char actual_checksum[OSTREE_SHA256_STRING_LEN+1]; - ot_checksum_get_hexdigest (&state->content_checksum, actual_checksum, sizeof (actual_checksum)); - - if (strcmp (actual_checksum, state->checksum) != 0) - return glnx_throw (error, "Corrupted object %s (actual checksum is %s)", - state->checksum, actual_checksum); - } - - if (!_ostree_repo_commit_trusted_content_bare (repo, state->checksum, &state->tmpf, - state->uid, state->gid, state->mode, - state->xattrs, - cancellable, error)) - return FALSE; - g_clear_object (&state->content_out); + g_assert_cmpstr (state->checksum, ==, actual_checksum); } if (!dispatch_unset_read_source (repo, state, cancellable, error)) return FALSE; g_clear_pointer (&state->xattrs, g_variant_unref); - ot_checksum_clear (&state->content_checksum); + _ostree_repo_bare_content_cleanup (&state->content_out); state->checksum_index++; state->output_target = NULL; From 277ce6b36dc05dbdfa3a01fccdcc3f7493d70e78 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 30 Nov 2017 20:43:02 -0500 Subject: [PATCH 38/46] tests/delta-crosscheck: Disable fsync I was running this recently to test the last delta write changes, and this helps. We should add an option to repo-init to make this easier at some point. Closes: #1356 Approved by: jlebon --- manual-tests/static-delta-generate-crosscheck.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/manual-tests/static-delta-generate-crosscheck.sh b/manual-tests/static-delta-generate-crosscheck.sh index 2eea3ff2..a9335007 100755 --- a/manual-tests/static-delta-generate-crosscheck.sh +++ b/manual-tests/static-delta-generate-crosscheck.sh @@ -43,6 +43,7 @@ assert_streq() { validate_delta_options() { ostree --repo=testrepo init --mode=bare-user + echo 'fsync=false' >> testrepo/config ostree --repo=testrepo remote add --set=gpg-verify=false local file://${repo} ostree --repo=${repo} static-delta generate $@ --from=${from} --to=${to} ostree --repo=${repo} summary -u From 72304a272c7df7870ce1cafb5e97b611fa2114a7 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 28 Nov 2017 06:14:29 -0500 Subject: [PATCH 39/46] lib/commit: Reuse txn dir for tmpfiles This closes a race condition I was seeing with `test-concurrency.py`. If we don't have `O_TMPFILE` (or for symlinks) we'll create temporary files; previously these would be subject to the date-based pruning because we set the timestamp to 0 for objects. Having our temporary files also in the txn staging dir ensures that they're covered by the locking we do for that directory, and it's also generally cleaner since the lifecycle of all the temporary data for a txn is in one place. Closes: #1352 Approved by: dbnicholson --- src/libostree/ostree-repo-commit.c | 32 +++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index e283702d..aaea0a06 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -51,6 +51,20 @@ commit_dest_dfd (OstreeRepo *self) return self->objects_dir_fd; } +/* If we don't have O_TMPFILE, or for symlinks we'll create temporary + * files. If we have a txn, use the staging dir to ensure that + * things are consistently locked against concurrent cleanup, and + * in general we have all of our data in one place. + */ +static int +commit_tmp_dfd (OstreeRepo *self) +{ + if (self->in_transaction) + return self->commit_stagedir.fd; + else + return self->tmp_dir_fd; +} + /* The objects/ directory has a two-character directory prefix for checksums * to avoid putting lots of files in a single directory. This technique * is quite old, but Git also uses it for example. @@ -439,7 +453,7 @@ _ostree_repo_bare_content_open (OstreeRepo *self, g_assert (!real->initialized); real->initialized = TRUE; g_assert (S_ISREG (mode)); - if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, + if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_WRONLY|O_CLOEXEC, &real->tmpf, error)) return FALSE; ot_checksum_init (&real->checksum); @@ -530,7 +544,7 @@ create_regular_tmpfile_linkable_with_content (OstreeRepo *self, GError **error) { g_auto(GLnxTmpfile) tmpf = { 0, }; - if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, + if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_WRONLY|O_CLOEXEC, &tmpf, error)) return FALSE; @@ -668,7 +682,7 @@ write_content_object (OstreeRepo *self, * * We use GLnxTmpfile for regular files, and OtCleanupUnlinkat for symlinks. */ - g_auto(OtCleanupUnlinkat) tmp_unlinker = { self->tmp_dir_fd, NULL }; + g_auto(OtCleanupUnlinkat) tmp_unlinker = { commit_tmp_dfd (self), NULL }; g_auto(GLnxTmpfile) tmpf = { 0, }; goffset unpacked_size = 0; gboolean indexable = FALSE; @@ -677,7 +691,7 @@ write_content_object (OstreeRepo *self, { /* This will not be hit for bare-user or archive */ g_assert (self->mode == OSTREE_REPO_MODE_BARE || self->mode == OSTREE_REPO_MODE_BARE_USER_ONLY); - if (!_ostree_make_temporary_symlink_at (self->tmp_dir_fd, + if (!_ostree_make_temporary_symlink_at (commit_tmp_dfd (self), g_file_info_get_symlink_target (file_info), &tmp_unlinker.path, cancellable, error)) @@ -700,7 +714,7 @@ write_content_object (OstreeRepo *self, if (self->generate_sizes) indexable = TRUE; - if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, + if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_WRONLY|O_CLOEXEC, &tmpf, error)) return FALSE; temp_out = g_unix_output_stream_new (tmpf.fd, FALSE); @@ -790,14 +804,14 @@ write_content_object (OstreeRepo *self, * Note, this does not apply for bare-user repos, as they store symlinks * as regular files. */ - if (G_UNLIKELY (fchownat (self->tmp_dir_fd, tmp_unlinker.path, + if (G_UNLIKELY (fchownat (tmp_unlinker.dfd, tmp_unlinker.path, uid, gid, AT_SYMLINK_NOFOLLOW) == -1)) return glnx_throw_errno_prefix (error, "fchownat"); if (xattrs != NULL) { - ot_security_smack_reset_dfd_name (self->tmp_dir_fd, tmp_unlinker.path); - if (!glnx_dfd_name_set_all_xattrs (self->tmp_dir_fd, tmp_unlinker.path, + ot_security_smack_reset_dfd_name (tmp_unlinker.dfd, tmp_unlinker.path); + if (!glnx_dfd_name_set_all_xattrs (tmp_unlinker.dfd, tmp_unlinker.path, xattrs, cancellable, error)) return FALSE; } @@ -1039,7 +1053,7 @@ write_metadata_object (OstreeRepo *self, /* Write the metadata to a temporary file */ g_auto(GLnxTmpfile) tmpf = { 0, }; - if (!glnx_open_tmpfile_linkable_at (self->tmp_dir_fd, ".", O_WRONLY|O_CLOEXEC, + if (!glnx_open_tmpfile_linkable_at (commit_tmp_dfd (self), ".", O_WRONLY|O_CLOEXEC, &tmpf, error)) return FALSE; if (!glnx_try_fallocate (tmpf.fd, 0, len, error)) From 870b614f37b17c9e2583e24ebae7467687015b20 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 28 Nov 2017 09:33:17 -0500 Subject: [PATCH 40/46] lib/commit: Minor refactoring of tmpdir cleanup code Prep for future work here; let's cleanly separate the path for cleaning up the txn staging directories from the code that cleans up "other stuff". Currently only the former case uses the `GLnxLockFile` etc. Closes: #1352 Approved by: dbnicholson --- src/libostree/ostree-repo-commit.c | 93 +++++++++++++++++------------- 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index aaea0a06..85e2e891 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -1441,6 +1441,41 @@ rename_pending_loose_objects (OstreeRepo *self, return TRUE; } +/* Try to lock a transaction stage directory created by + * ostree_repo_prepare_transaction(). + */ +static gboolean +cleanup_txn_dir (OstreeRepo *self, + int dfd, + const char *path, + GCancellable *cancellable, + GError **error) +{ + g_auto(GLnxLockFile) lockfile = { 0, }; + gboolean did_lock; + + /* Try to lock, but if we don't get it, move on */ + if (!_ostree_repo_try_lock_tmpdir (dfd, path, &lockfile, &did_lock, error)) + return FALSE; + if (!did_lock) + return TRUE; /* Note early return */ + + /* If however this is the staging directory for the *current* + * boot, then don't delete it now - we may end up reusing it, as + * is the point. + */ + if (g_str_has_prefix (path, self->stagedir_prefix)) + return TRUE; /* Note early return */ + + /* But, crucially we can now clean up staging directories + * from *other* boots. + */ + if (!glnx_shutil_rm_rf_at (dfd, path, cancellable, error)) + return glnx_prefix_error (error, "Removing %s", path); + + return TRUE; +} + /* Look in repo/tmp and delete files that are older than a day (by default). * This used to be primarily used by the libsoup fetcher which stored partially * written objects. In practice now that that isn't done anymore, we should @@ -1461,15 +1496,9 @@ cleanup_tmpdir (OstreeRepo *self, while (TRUE) { - guint64 delta; struct dirent *dent; - struct stat stbuf; - g_auto(GLnxLockFile) lockfile = { 0, }; - gboolean did_lock; - if (!glnx_dirfd_iterator_next_dent (&dfd_iter, &dent, cancellable, error)) return FALSE; - if (dent == NULL) break; @@ -1479,56 +1508,40 @@ cleanup_tmpdir (OstreeRepo *self, if (strcmp (dent->d_name, "cache") == 0) continue; + struct stat stbuf; if (!glnx_fstatat_allow_noent (dfd_iter.fd, dent->d_name, &stbuf, AT_SYMLINK_NOFOLLOW, error)) return FALSE; if (errno == ENOENT) /* Did another cleanup win? */ continue; - /* First, if it's a directory which needs locking, but it's - * busy, skip it. - */ + /* Handle transaction tmpdirs */ if (_ostree_repo_is_locked_tmpdir (dent->d_name)) { - if (!_ostree_repo_try_lock_tmpdir (dfd_iter.fd, dent->d_name, - &lockfile, &did_lock, error)) + if (!cleanup_txn_dir (self, dfd_iter.fd, dent->d_name, cancellable, error)) return FALSE; - if (!did_lock) - continue; + continue; /* We've handled this, move on */ } - /* If however this is the staging directory for the *current* - * boot, then don't delete it now - we may end up reusing it, as - * is the point. + /* At this point we're looking at an unknown-origin file or directory in + * the tmpdir. This could be something like a temporary checkout dir (used + * by rpm-ostree), or (from older versions of libostree) a tempfile if we + * don't have O_TMPFILE for commits. */ - if (g_str_has_prefix (dent->d_name, self->stagedir_prefix)) + + /* Ignore files from the future */ + if (stbuf.st_mtime > curtime_secs) continue; - else if (g_str_has_prefix (dent->d_name, OSTREE_REPO_TMPDIR_STAGING)) + + /* We're pruning content based on the expiry, which + * defaults to a day. That's what we were doing before we + * had locking...but in future we can be smarter here. + */ + guint64 delta = curtime_secs - stbuf.st_mtime; + if (delta > self->tmp_expiry_seconds) { - /* But, crucially we can now clean up staging directories - * from *other* boots - */ if (!glnx_shutil_rm_rf_at (dfd_iter.fd, dent->d_name, cancellable, error)) return glnx_prefix_error (error, "Removing %s", dent->d_name); } - else - { - /* Now we do time-based cleanup. Ignore it if it's somehow - * in the future... - */ - if (stbuf.st_mtime > curtime_secs) - continue; - - /* Now, we're pruning content based on the expiry, which - * defaults to a day. That's what we were doing before we - * had locking...but in future we can be smarter here. - */ - delta = curtime_secs - stbuf.st_mtime; - if (delta > self->tmp_expiry_seconds) - { - if (!glnx_shutil_rm_rf_at (dfd_iter.fd, dent->d_name, cancellable, error)) - return glnx_prefix_error (error, "Removing %s", dent->d_name); - } - } } return TRUE; From afebb5c336bebe4f41704ec995bb430d006f85e4 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Fri, 13 Oct 2017 19:52:04 -0500 Subject: [PATCH 41/46] tests: Run python tests with stdout unbuffered Set the PYTHONUNBUFFERED environment variable during tests so that python leaves stdout unbuffered. This is helpful when reading logs for failures since the interleaved stdout and stderr will generally come out in the right order. It's not perfect since tap-driver.sh does some special redirection to the log file, but it's an improvement. Closes: #1352 Approved by: dbnicholson --- Makefile-tests.am | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile-tests.am b/Makefile-tests.am index 164717b1..6d0e0865 100644 --- a/Makefile-tests.am +++ b/Makefile-tests.am @@ -35,6 +35,7 @@ AM_TESTS_ENVIRONMENT += OT_TESTS_DEBUG=1 \ LD_LIBRARY_PATH=$$(cd $(top_builddir)/.libs && pwd)$${LD_LIBRARY_PATH:+:$${LD_LIBRARY_PATH}} \ PATH=$$(cd $(top_builddir)/tests && pwd):$${PATH} \ OSTREE_FEATURES="$(OSTREE_FEATURES)" \ + PYTHONUNBUFFERED=1 \ $(NULL) if BUILDOPT_ASAN AM_TESTS_ENVIRONMENT += OT_SKIP_READDIR_RAND=1 G_SLICE=always-malloc From 681a62b92c80ee25fb01e2917b730e00960b8340 Mon Sep 17 00:00:00 2001 From: Dan Nicholson Date: Thu, 16 Nov 2017 17:27:14 +0000 Subject: [PATCH 42/46] ci: Really show test-suite.log on travis The test-suite.log file is created in the top directory, not in test (which isn't even a directory here). Closes: #1352 Approved by: dbnicholson --- ci/travis-build.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ci/travis-build.sh b/ci/travis-build.sh index 885e3ce2..3fd969bd 100755 --- a/ci/travis-build.sh +++ b/ci/travis-build.sh @@ -90,9 +90,9 @@ make="make -j${ci_parallel} V=1 VERBOSE=1" ${make} [ "$ci_test" = no ] || ${make} check || maybe_fail_tests -cat test/test-suite.log || : +cat test-suite.log || : [ "$ci_test" = no ] || ${make} distcheck || maybe_fail_tests -cat test/test-suite.log || : +cat test-suite.log || : ${make} install DESTDIR=$(pwd)/DESTDIR ( cd DESTDIR && find . ) @@ -104,7 +104,7 @@ if [ "$ci_sudo" = yes ] && [ "$ci_test" = yes ]; then GI_TYPELIB_PATH=/usr/local/lib/girepository-1.0 \ ${make} installcheck || \ maybe_fail_tests - cat test/test-suite.log || : + cat test-suite.log || : fi # vim:set sw=4 sts=4 et: From 4eae6529ed478e9a6fa55fc43f127043ff4db287 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 28 Nov 2017 13:01:46 -0500 Subject: [PATCH 43/46] lib/commit: Move txn stagedir deletion/unlock into one place Previously we'd delete the tmpdir in `rename_pending_loose_objects()` but do the unlock inside `ostree_repo_commit_transaction()`. Move them into the same place in the latter function for consistency. Doesn't fix anything, just a cleanup while reading the code and working on `test-concurrency.py`. Closes: #1352 Approved by: dbnicholson --- src/libostree/ostree-repo-commit.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index 85e2e891..e2fdf9f4 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -1435,9 +1435,6 @@ rename_pending_loose_objects (OstreeRepo *self, return glnx_throw_errno_prefix (error, "fsync"); } - if (!glnx_tmpdir_delete (&self->commit_stagedir, cancellable, error)) - return FALSE; - return TRUE; } @@ -1773,6 +1770,12 @@ ostree_repo_commit_transaction (OstreeRepo *self, if (!rename_pending_loose_objects (self, cancellable, error)) return FALSE; + g_debug ("txn commit %s", glnx_basename (self->commit_stagedir.path)); + if (!glnx_tmpdir_delete (&self->commit_stagedir, cancellable, error)) + return FALSE; + glnx_release_lock_file (&self->commit_stagedir_lock); + + /* This performs a global cleanup */ if (!cleanup_tmpdir (self, cancellable, error)) return FALSE; @@ -1789,9 +1792,6 @@ ostree_repo_commit_transaction (OstreeRepo *self, return FALSE; g_clear_pointer (&self->txn_collection_refs, g_hash_table_destroy); - glnx_tmpdir_unset (&self->commit_stagedir); - glnx_release_lock_file (&self->commit_stagedir_lock); - self->in_transaction = FALSE; if (!ot_ensure_unlinked_at (self->repo_dir_fd, "transaction", 0)) From 5ef8faff9a10f055401df5265e389ba9bbb89786 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 28 Nov 2017 13:03:00 -0500 Subject: [PATCH 44/46] lib/repo: Verify txn stagedir existence after locking This squashes the last race condition I was actively hitting while running `test-concurrency.py` in a loop. The race is when process A finds a tmpdir to reuse, and goes to lock it. Meanwhile process B deletes it and unlocks the lock. Process A then succeeds at grabbing a lock, but the tmpdir is deleted. Closes: #1352 Approved by: dbnicholson --- src/libostree/ostree-repo.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index 6e342125..afd56e4c 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -4904,7 +4904,16 @@ _ostree_repo_try_lock_tmpdir (int tmpdir_dfd, } else { - did_lock = TRUE; + /* It's possible that we got a lock after seeing the directory, but + * another process deleted the tmpdir, so verify it still exists. + */ + struct stat stbuf; + if (!glnx_fstatat_allow_noent (tmpdir_dfd, tmpdir_name, &stbuf, AT_SYMLINK_NOFOLLOW, error)) + return FALSE; + if (errno == 0 && S_ISDIR (stbuf.st_mode)) + did_lock = TRUE; + else + glnx_release_lock_file (file_lock_out); } *out_did_lock = did_lock; From 7c8ea25306a39c211a877dc134f0a393191a0193 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 30 Nov 2017 21:43:17 -0500 Subject: [PATCH 45/46] lib/repo: Add a DEVINO_CANONICAL commit modifier flag I was seeing the `Writing OSTree commit...` phase of rpm-ostree being very slow lately. This turns out to be more fallout from https://github.com/ostreedev/ostree/pull/1170 AKA commit: 8fe4536 Loading the xattrs is slow on my system (F27AW, XFS+LVM, NVMe). I haven't fully traced through why, but AIUI at least on XFS the xattrs are often stored outside of the inode so it's a little bit like doing an `open()+read()`. Plus there's the LSM overhead, etc. The thing is that for rpm-ostree's package layering use case, we basically always want to treat the on-disk state as canonical. (There's a subtle case here if one does overrides for something that contains policy but we'll fix that). Anyways, so we're in a state now where we do the slow but correct thing by default, which seems sane. But let's allow the app to opt-in to telling us "really trust devino". The difference between a `stat()` + hash table lookup versus the full xattr load on my test case of `rpm-ostree install ./tree-1.7.0-10.fc27.x86_64.rpm` is absolutely dramatic; consistently on the order of 10s without this support, and <1s with (800ms). Closes: #1357 Approved by: jlebon --- src/libostree/ostree-repo-commit.c | 87 +++++++++++++++++++----------- src/libostree/ostree-repo.h | 2 + src/ostree/ot-builtin-commit.c | 7 +++ tests/test-basic-user.sh | 22 +++++++- 4 files changed, 85 insertions(+), 33 deletions(-) diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index e2fdf9f4..a286e7ad 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2793,47 +2793,70 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, g_assert (dir_enum != NULL || dfd_iter != NULL); GFileType file_type = g_file_info_get_file_type (child_info); - const char *name = g_file_info_get_name (child_info); - g_ptr_array_add (path, (char*)name); - g_autofree char *child_relpath = ptrarray_path_join (path); - - /* See if we have a devino hit; this is used below. Further, for bare-user - * repos we'll reload our file info from the object (specifically the - * ostreemeta xattr). The on-disk state is not normally what we want to - * commit. Basically we're making sure that we pick up "real" uid/gid and any - * xattrs there. + /* Load flags into boolean constants for ease of readability (we also need to + * NULL-check modifier) */ - const char *loose_checksum = NULL; - g_autoptr(GVariant) source_xattrs = NULL; - if (dfd_iter != NULL && (file_type != G_FILE_TYPE_DIRECTORY)) - { - guint32 dev = g_file_info_get_attribute_uint32 (child_info, "unix::device"); - guint64 inode = g_file_info_get_attribute_uint64 (child_info, "unix::inode"); - loose_checksum = devino_cache_lookup (self, modifier, dev, inode); - if (loose_checksum && self->mode == OSTREE_REPO_MODE_BARE_USER) - { - child_info = NULL; - if (!ostree_repo_load_file (self, loose_checksum, NULL, &child_info, &source_xattrs, - cancellable, error)) - return FALSE; - } - } - - g_autoptr(GFileInfo) modified_info = NULL; - OstreeRepoCommitFilterResult filter_result = - _ostree_repo_commit_modifier_apply (self, modifier, child_relpath, child_info, &modified_info); - const gboolean child_info_was_modified = !_ostree_gfileinfo_equal (child_info, modified_info); - + const gboolean canonical_permissions = modifier && + (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS); + const gboolean devino_canonical = modifier && + (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL); /* We currently only honor the CONSUME flag in the dfd_iter case to avoid even * more complexity in this function, and it'd mostly only be useful when * operating on local filesystems anyways. */ const gboolean delete_after_commit = dfd_iter && modifier && (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME); - const gboolean canonical_permissions = modifier && - (modifier->flags & OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS); + + /* See if we have a devino hit; this is used below in a few places. */ + const char *loose_checksum = NULL; + if (dfd_iter != NULL && (file_type != G_FILE_TYPE_DIRECTORY)) + { + guint32 dev = g_file_info_get_attribute_uint32 (child_info, "unix::device"); + guint64 inode = g_file_info_get_attribute_uint64 (child_info, "unix::inode"); + loose_checksum = devino_cache_lookup (self, modifier, dev, inode); + if (loose_checksum && devino_canonical) + { + /* Go directly to checksum, do not pass Go, do not collect $200. + * In this mode the app is required to break hardlinks for any + * files it wants to modify. + */ + if (!ostree_mutable_tree_replace_file (mtree, name, loose_checksum, error)) + return FALSE; + if (delete_after_commit) + { + if (!glnx_shutil_rm_rf_at (dfd_iter->fd, name, cancellable, error)) + return FALSE; + } + return TRUE; /* Early return */ + } + } + + /* Build the full path which we need for callbacks */ + g_ptr_array_add (path, (char*)name); + g_autofree char *child_relpath = ptrarray_path_join (path); + + /* For bare-user repos we'll reload our file info from the object + * (specifically the ostreemeta xattr), if it was checked out that way (via + * hardlink). The on-disk state is not normally what we want to commit. + * Basically we're making sure that we pick up "real" uid/gid and any xattrs + * there. + */ + g_autoptr(GVariant) source_xattrs = NULL; + if (loose_checksum && self->mode == OSTREE_REPO_MODE_BARE_USER) + { + child_info = NULL; + if (!ostree_repo_load_file (self, loose_checksum, NULL, &child_info, &source_xattrs, + cancellable, error)) + return FALSE; + } + + /* Call the filter */ + g_autoptr(GFileInfo) modified_info = NULL; + OstreeRepoCommitFilterResult filter_result = + _ostree_repo_commit_modifier_apply (self, modifier, child_relpath, child_info, &modified_info); + const gboolean child_info_was_modified = !_ostree_gfileinfo_equal (child_info, modified_info); if (filter_result != OSTREE_REPO_COMMIT_FILTER_ALLOW) { diff --git a/src/libostree/ostree-repo.h b/src/libostree/ostree-repo.h index bec43c30..db54f022 100644 --- a/src/libostree/ostree-repo.h +++ b/src/libostree/ostree-repo.h @@ -644,6 +644,7 @@ typedef OstreeRepoCommitFilterResult (*OstreeRepoCommitFilter) (OstreeRepo *r * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS: Canonicalize permissions for bare-user-only mode. * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_ERROR_ON_UNLABELED: Emit an error if configured SELinux policy does not provide a label * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME: Delete added files/directories after commit; Since: 2017.13 + * @OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL: If a devino cache hit is found, skip modifier filters (non-directories only); Since: 2017.14 */ typedef enum { OSTREE_REPO_COMMIT_MODIFIER_FLAGS_NONE = 0, @@ -652,6 +653,7 @@ typedef enum { OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS = (1 << 2), OSTREE_REPO_COMMIT_MODIFIER_FLAGS_ERROR_ON_UNLABELED = (1 << 3), OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME = (1 << 4), + OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL = (1 << 5), } OstreeRepoCommitModifierFlags; /** diff --git a/src/ostree/ot-builtin-commit.c b/src/ostree/ot-builtin-commit.c index a8eb79aa..c24e06c7 100644 --- a/src/ostree/ot-builtin-commit.c +++ b/src/ostree/ot-builtin-commit.c @@ -51,6 +51,7 @@ static gboolean opt_no_xattrs; static char *opt_selinux_policy; static gboolean opt_canonical_permissions; static gboolean opt_consume; +static gboolean opt_devino_canonical; static char **opt_trees; static gint opt_owner_uid = -1; static gint opt_owner_gid = -1; @@ -98,6 +99,7 @@ static GOptionEntry options[] = { { "no-xattrs", 0, 0, G_OPTION_ARG_NONE, &opt_no_xattrs, "Do not import extended attributes", NULL }, { "selinux-policy", 0, 0, G_OPTION_ARG_FILENAME, &opt_selinux_policy, "Set SELinux labels based on policy in root filesystem PATH (may be /)", "PATH" }, { "link-checkout-speedup", 0, 0, G_OPTION_ARG_NONE, &opt_link_checkout_speedup, "Optimize for commits of trees composed of hardlinks into the repository", NULL }, + { "devino-canonical", 'I', 0, G_OPTION_ARG_NONE, &opt_devino_canonical, "Assume hardlinked objects are unmodified. Implies --link-checkout-speedup", NULL }, { "tar-autocreate-parents", 0, 0, G_OPTION_ARG_NONE, &opt_tar_autocreate_parents, "When loading tar archives, automatically create parent directories as needed", NULL }, { "tar-pathname-filter", 0, 0, G_OPTION_ARG_STRING, &opt_tar_pathname_filter, "When loading tar archives, use REGEX,REPLACEMENT against path names", "REGEX,REPLACEMENT" }, { "skip-if-unchanged", 0, 0, G_OPTION_ARG_NONE, &opt_skip_if_unchanged, "If the contents are unchanged from previous commit, do nothing", NULL }, @@ -480,6 +482,11 @@ ostree_builtin_commit (int argc, char **argv, OstreeCommandInvocation *invocatio flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_SKIP_XATTRS; if (opt_consume) flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CONSUME; + if (opt_devino_canonical) + { + opt_link_checkout_speedup = TRUE; /* Imply this */ + flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_DEVINO_CANONICAL; + } if (opt_canonical_permissions) flags |= OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS; if (opt_generate_sizes) diff --git a/tests/test-basic-user.sh b/tests/test-basic-user.sh index bc08b65a..7f970b5c 100755 --- a/tests/test-basic-user.sh +++ b/tests/test-basic-user.sh @@ -25,7 +25,7 @@ skip_without_user_xattrs setup_test_repository "bare-user" -extra_basic_tests=5 +extra_basic_tests=6 . $(dirname $0)/basic-test.sh # Reset things so we don't inherit a lot of state from earlier tests @@ -99,3 +99,23 @@ assert_file_has_content ls.txt '^-007.. 0 0 .*/usr/bin/systemd' $OSTREE ls rootfs /usr/lib/dbus-daemon-helper >ls.txt assert_file_has_content ls.txt '^-007.. 0 81 .*/usr/lib/dbus-daemon-helper' echo "ok bare-user link-checkout-speedup maintains uids" + +cd ${test_tmpdir} +rm -rf test2-checkout +$OSTREE checkout -H -U test2 test2-checkout +# With --link-checkout-speedup, specifying --owner-uid should "win" by default. +myid=$(id -u) +newid=$((${myid} + 1)) +$OSTREE commit ${COMMIT_ARGS} --owner-uid ${newid} --owner-gid ${newid} \ + --link-checkout-speedup -b test2-linkcheckout-test --tree=dir=test2-checkout +$OSTREE ls test2-linkcheckout-test /baz/cow > ls.txt +assert_file_has_content ls.txt "^-006.. ${newid} ${newid} .*/baz/cow" + +# But --devino-canonical should override that +$OSTREE commit ${COMMIT_ARGS} --owner-uid ${newid} --owner-gid ${newid} \ + -I -b test2-devino-test --tree=dir=test2-checkout +$OSTREE ls test2-devino-test /baz/cow > ls.txt +assert_file_has_content ls.txt "^-006.. ${myid} ${myid} .*/baz/cow" + +$OSTREE refs --delete test2-{linkcheckout,devino}-test +echo "ok commit with -I" From b0f9a298165a99d5621073bd9e1b10f13058e838 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Mon, 4 Dec 2017 11:06:58 -0500 Subject: [PATCH 46/46] Release 2017.14 Time to cut a new release, we've got the libcurl cleanup ordering patch which several people have hit, along with safe early fixes for tmpdir cleanup. Let's try to land the locking PR early next cycle. Closes: #1359 Approved by: jlebon --- configure.ac | 2 +- src/libostree/libostree-devel.sym | 5 +++-- src/libostree/libostree-released.sym | 3 +++ tests/test-symbols.sh | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index a1271909..629c923b 100644 --- a/configure.ac +++ b/configure.ac @@ -7,7 +7,7 @@ m4_define([year_version], [2017]) m4_define([release_version], [14]) 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 3c5da23e..df18b9ab 100644 --- a/src/libostree/libostree-devel.sym +++ b/src/libostree/libostree-devel.sym @@ -18,8 +18,9 @@ ***/ /* Add new symbols here. Release commits should copy this section into -released.sym. */ -LIBOSTREE_2017.14 { -} LIBOSTREE_2017.13; + +LIBOSTREE_2017.15 { +} LIBOSTREE_2017.14; /* 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 diff --git a/src/libostree/libostree-released.sym b/src/libostree/libostree-released.sym index 4cab4e5d..e4d50895 100644 --- a/src/libostree/libostree-released.sym +++ b/src/libostree/libostree-released.sym @@ -442,6 +442,9 @@ global: ostree_repo_checkout_at_options_set_devino; } LIBOSTREE_2017.12; +LIBOSTREE_2017.14 { +} LIBOSTREE_2017.13; + /* 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 0bde00af..6e71e2dd 100755 --- a/tests/test-symbols.sh +++ b/tests/test-symbols.sh @@ -52,7 +52,7 @@ echo 'ok documented symbols' # ONLY update this checksum in release commits! cat > released-sha256.txt <