From 1eff3e83436b6129c0dc350dbbda52ba330e3834 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 30 May 2017 14:07:13 -0400 Subject: [PATCH] Add a notion of "physical" sysroot, use for remote writing Using `${sysroot}` to mean the physical storage root: We don't want to write to `${sysroot}/etc/ostree/remotes.d`, since nothing will read it, and really `${sysroot}` should just have `/ostree` (ideally). Today the Anaconda rpmostree code ends up writing there. Fix this by adding a notion of "physical" sysroot. We determine whether the path is physical by checking for `/sysroot`, which exists in deployment roots (and there shouldn't be a `${sysroot}/sysroot`). In order to unit test this, I added a `--sysroot` argument to `remote add`. However, doing this better would require reworking the command line parsing for the `remote` argument to support specifying `--repo` or `--sysroot`, and I didn't quite want to do that yet in this patch. Closes: https://github.com/ostreedev/ostree/issues/892 Closes: #896 Approved by: jlebon --- src/libostree/ostree-repo-private.h | 1 + src/libostree/ostree-repo.c | 39 +++++++++++++------------- src/libostree/ostree-sysroot-private.h | 3 +- src/libostree/ostree-sysroot.c | 22 +++++++++++++++ src/ostree/ot-remote-builtin-add.c | 17 +++++++++++ tests/admin-test.sh | 16 +++++++++++ tests/test-admin-deploy-grub2.sh | 2 +- tests/test-admin-deploy-syslinux.sh | 2 +- tests/test-admin-deploy-uboot.sh | 2 +- 9 files changed, 81 insertions(+), 23 deletions(-) diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 015a9a8b..6a7e9ee8 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -93,6 +93,7 @@ struct OstreeRepo { int objects_dir_fd; int uncompressed_objects_dir_fd; GFile *sysroot_dir; + GWeakRef sysroot; /* Weak to avoid a circular ref; see also `is_system` */ char *remotes_config_dir; GHashTable *txn_refs; diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c index cbbaec9b..4ad112df 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -32,6 +32,7 @@ #include #include "ostree-core-private.h" +#include "ostree-sysroot-private.h" #include "ostree-remote-private.h" #include "ostree-repo-private.h" #include "ostree-repo-file.h" @@ -447,6 +448,7 @@ ostree_repo_finalize (GObject *object) if (self->uncompressed_objects_dir_fd != -1) (void) close (self->uncompressed_objects_dir_fd); g_clear_object (&self->sysroot_dir); + g_weak_ref_clear (&self->sysroot); g_free (self->remotes_config_dir); if (self->loose_object_devino_hash) @@ -525,10 +527,6 @@ ostree_repo_constructed (GObject *object) g_assert (self->repodir != NULL); - /* Ensure the "sysroot-path" property is set. */ - if (self->sysroot_dir == NULL) - self->sysroot_dir = g_object_ref (_ostree_get_default_sysroot_path ()); - G_OBJECT_CLASS (ostree_repo_parent_class)->constructed (object); } @@ -706,8 +704,6 @@ ostree_repo_new_default (void) gboolean ostree_repo_is_system (OstreeRepo *repo) { - g_autoptr(GFile) default_repo_path = NULL; - g_return_val_if_fail (OSTREE_IS_REPO (repo), FALSE); /* If we were created via ostree_sysroot_get_repo(), we know the answer is yes @@ -716,8 +712,11 @@ ostree_repo_is_system (OstreeRepo *repo) if (repo->is_system) return TRUE; - default_repo_path = get_default_repo_path (repo->sysroot_dir); + /* No sysroot_dir set? Not a system repo then. */ + if (!repo->sysroot_dir) + return FALSE; + g_autoptr(GFile) default_repo_path = get_default_repo_path (repo->sysroot_dir); return g_file_equal (repo->repodir, default_repo_path); } @@ -894,23 +893,25 @@ impl_repo_remote_add (OstreeRepo *self, remote = ostree_remote_new (name); - /* The OstreeRepo maintains its own internal system root path, - * so we need to not only check if a "sysroot" argument was given - * but also whether it's actually different from OstreeRepo's. - * - * XXX Having API regret about the "sysroot" argument now. + /* If a sysroot was provided, use it. Otherwise, see if this repo has a ref to + * a sysroot (and it's physical). */ - gboolean different_sysroot = FALSE; - if (sysroot != NULL) - different_sysroot = !g_file_equal (sysroot, self->sysroot_dir); + g_autoptr(OstreeSysroot) sysroot_ref = NULL; + if (sysroot == NULL) + { + sysroot_ref = (OstreeSysroot*)g_weak_ref_get (&self->sysroot); + /* Only write to /etc/ostree/remotes.d if we are pointed at a deployment */ + if (sysroot_ref != NULL && !sysroot_ref->is_physical) + sysroot = ostree_sysroot_get_path (sysroot_ref); + } + /* For backwards compat, also fall back to the sysroot-path variable */ + if (sysroot == NULL && sysroot_ref == NULL) + sysroot = self->sysroot_dir; - if (different_sysroot || ostree_repo_is_system (self)) + if (sysroot != NULL) { g_autoptr(GError) local_error = NULL; - if (sysroot == NULL) - sysroot = self->sysroot_dir; - g_autoptr(GFile) etc_ostree_remotes_d = g_file_resolve_relative_path (sysroot, SYSCONF_REMOTES); if (!g_file_make_directory_with_parents (etc_ostree_remotes_d, cancellable, &local_error)) diff --git a/src/libostree/ostree-sysroot-private.h b/src/libostree/ostree-sysroot-private.h index 14ee5cad..82abc8e7 100644 --- a/src/libostree/ostree-sysroot-private.h +++ b/src/libostree/ostree-sysroot-private.h @@ -48,7 +48,8 @@ struct OstreeSysroot { GLnxLockFile lock; gboolean loaded; - + + gboolean is_physical; /* TRUE if we're pointed at physical storage root and not a deployment */ GPtrArray *deployments; int bootversion; int subbootversion; diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c index 86aa8ce3..b8f494bf 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -135,6 +135,8 @@ ostree_sysroot_constructed (GObject *object) repo_path = g_file_resolve_relative_path (self->path, "ostree/repo"); self->repo = ostree_repo_new_for_sysroot_path (repo_path, self->path); self->repo->is_system = TRUE; + /* Hold a weak ref for the remote-add handling */ + g_weak_ref_init (&self->repo->sysroot, object); G_OBJECT_CLASS (ostree_sysroot_parent_class)->constructed (object); } @@ -813,6 +815,26 @@ ostree_sysroot_load_if_changed (OstreeSysroot *self, cancellable, error)) return FALSE; + /* Determine whether we're "physical" or not, the first time we initialize */ + if (!self->loaded) + { + /* If we have a booted deployment, the sysroot is / and we're definitely + * not physical. + */ + if (self->booted_deployment) + self->is_physical = FALSE; /* (the default, but explicit for clarity) */ + /* Otherwise - check for /sysroot which should only exist in a deployment, + * not in ${sysroot} (a metavariable for the real physical root). + */ + else if (fstatat (self->sysroot_fd, "sysroot", &stbuf, 0) < 0) + { + if (errno != ENOENT) + return glnx_throw_errno_prefix (error, "fstatat"); + self->is_physical = TRUE; + } + /* Otherwise, the default is FALSE */ + } + self->bootversion = bootversion; self->subbootversion = subbootversion; self->deployments = deployments; diff --git a/src/ostree/ot-remote-builtin-add.c b/src/ostree/ot-remote-builtin-add.c index 3e3aeda9..9639b4a5 100644 --- a/src/ostree/ot-remote-builtin-add.c +++ b/src/ostree/ot-remote-builtin-add.c @@ -31,6 +31,7 @@ static gboolean opt_no_gpg_verify; static gboolean opt_if_not_exists; static char *opt_gpg_import; static char *opt_contenturl; +static char *opt_sysroot; static GOptionEntry option_entries[] = { { "set", 0, 0, G_OPTION_ARG_STRING_ARRAY, &opt_set, "Set config option KEY=VALUE for remote", "KEY=VALUE" }, @@ -38,6 +39,7 @@ static GOptionEntry option_entries[] = { { "if-not-exists", 0, 0, G_OPTION_ARG_NONE, &opt_if_not_exists, "Do nothing if the provided remote exists", NULL }, { "gpg-import", 0, 0, G_OPTION_ARG_FILENAME, &opt_gpg_import, "Import GPG key from FILE", "FILE" }, { "contenturl", 0, 0, G_OPTION_ARG_STRING, &opt_contenturl, "Use URL when fetching content", "URL" }, + { "sysroot", 0, 0, G_OPTION_ARG_FILENAME, &opt_sysroot, "Use sysroot at PATH (overrides --repo)", "PATH" }, { NULL } }; @@ -51,6 +53,7 @@ ot_remote_builtin_add (int argc, char **argv, GCancellable *cancellable, GError char **iter; g_autoptr(GVariantBuilder) optbuilder = NULL; g_autoptr(GVariant) options = NULL; + g_autoptr(OstreeSysroot) sysroot = NULL; gboolean ret = FALSE; context = g_option_context_new ("NAME [metalink=|mirrorlist=]URL [BRANCH...] - Add a remote repository"); @@ -59,6 +62,20 @@ ot_remote_builtin_add (int argc, char **argv, GCancellable *cancellable, GError OSTREE_BUILTIN_FLAG_NONE, &repo, cancellable, error)) goto out; + /* As a special case, we can take a --sysroot argument. Currently we also + * require --repo because fixing that needs more cmdline rework. + */ + if (opt_sysroot) + { + g_clear_object (&repo); + g_autoptr(GFile) sysroot_path = g_file_new_for_path (opt_sysroot); + sysroot = ostree_sysroot_new (sysroot_path); + if (!ostree_sysroot_load (sysroot, cancellable, error)) + goto out; + if (!ostree_sysroot_get_repo (sysroot, &repo, cancellable, error)) + goto out; + } + if (argc < 3) { ot_util_usage_error (context, "NAME and URL must be specified", error); diff --git a/tests/admin-test.sh b/tests/admin-test.sh index cc06fe6f..7182e60b 100644 --- a/tests/admin-test.sh +++ b/tests/admin-test.sh @@ -232,3 +232,19 @@ curr_rev=$(${CMD_PREFIX} ostree rev-parse --repo=sysroot/ostree/repo testos/buil assert_streq ${curr_rev} ${head_rev} echo "ok upgrade with and without override-commit" + +deployment=$(${CMD_PREFIX} ostree admin --sysroot=sysroot --print-current-dir) +${CMD_PREFIX} ostree --repo=sysroot/ostree/repo --sysroot=sysroot remote add --set=gpg-verify=false remote-test-physical file://$(pwd)/testos-repo +assert_not_has_file ${deployment}/etc/ostree/remotes.d/remote-test-physical.conf testos-repo +assert_file_has_content sysroot/ostree/repo/config remote-test-physical +echo "ok remote add physical sysroot" + +# Now a hack...symlink ${deployment}/sysroot to the sysroot in lieu of a bind +# mount which we can't do in unit tests. +ln -sr sysroot ${deployment}/sysroot +ln -s sysroot/ostree ${deployment}/ostree +ls -al ${deployment} +${CMD_PREFIX} ostree --repo=sysroot/ostree/repo --sysroot=${deployment} remote add --set=gpg-verify=false remote-test-nonphysical file://$(pwd)/testos-repo +assert_not_file_has_content sysroot/ostree/repo/config remote-test-nonphysical +assert_file_has_content ${deployment}/etc/ostree/remotes.d/remote-test-nonphysical.conf testos-repo +echo "ok remote add nonphysical sysroot" diff --git a/tests/test-admin-deploy-grub2.sh b/tests/test-admin-deploy-grub2.sh index 2b90c286..d7c1c6db 100755 --- a/tests/test-admin-deploy-grub2.sh +++ b/tests/test-admin-deploy-grub2.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..16" +echo "1..18" . $(dirname $0)/libtest.sh diff --git a/tests/test-admin-deploy-syslinux.sh b/tests/test-admin-deploy-syslinux.sh index 70b3b4d3..797836f0 100755 --- a/tests/test-admin-deploy-syslinux.sh +++ b/tests/test-admin-deploy-syslinux.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..16" +echo "1..18" . $(dirname $0)/libtest.sh diff --git a/tests/test-admin-deploy-uboot.sh b/tests/test-admin-deploy-uboot.sh index d4c3a0db..d9104f8c 100755 --- a/tests/test-admin-deploy-uboot.sh +++ b/tests/test-admin-deploy-uboot.sh @@ -20,7 +20,7 @@ set -euo pipefail -echo "1..17" +echo "1..19" . $(dirname $0)/libtest.sh