From cad42d960150db19bfaa518a06cea1febeb8fb58 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 2 Jun 2017 09:27:52 -0400 Subject: [PATCH] Revert "Add a notion of "physical" sysroot, use for remote writing" This reverts commit 1eff3e83436b6129c0dc350dbbda52ba330e3834. There are a few issues with it. It's not a critical thing for now, so let's ugly up the git history and revisit when we have time to debug it and add more tests. Besides the below issue, I noticed that the simple `ostree remote add` now writes to `/ostree/repo/config` because we *aren't* using the `--sysroot` argument. Closes: https://github.com/ostreedev/ostree/issues/901 Closes: #902 Approved by: mike-nguyen --- 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, 23 insertions(+), 81 deletions(-) diff --git a/src/libostree/ostree-repo-private.h b/src/libostree/ostree-repo-private.h index 67c9e44c..55602940 100644 --- a/src/libostree/ostree-repo-private.h +++ b/src/libostree/ostree-repo-private.h @@ -93,7 +93,6 @@ 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 4ad112df..cbbaec9b 100644 --- a/src/libostree/ostree-repo.c +++ b/src/libostree/ostree-repo.c @@ -32,7 +32,6 @@ #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" @@ -448,7 +447,6 @@ 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) @@ -527,6 +525,10 @@ 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); } @@ -704,6 +706,8 @@ 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 @@ -712,11 +716,8 @@ ostree_repo_is_system (OstreeRepo *repo) if (repo->is_system) return TRUE; - /* No sysroot_dir set? Not a system repo then. */ - if (!repo->sysroot_dir) - return FALSE; + default_repo_path = get_default_repo_path (repo->sysroot_dir); - g_autoptr(GFile) default_repo_path = get_default_repo_path (repo->sysroot_dir); return g_file_equal (repo->repodir, default_repo_path); } @@ -893,25 +894,23 @@ impl_repo_remote_add (OstreeRepo *self, remote = ostree_remote_new (name); - /* If a sysroot was provided, use it. Otherwise, see if this repo has a ref to - * a sysroot (and it's physical). + /* 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. */ - 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; - + gboolean different_sysroot = FALSE; if (sysroot != NULL) + different_sysroot = !g_file_equal (sysroot, self->sysroot_dir); + + if (different_sysroot || ostree_repo_is_system (self)) { 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 82abc8e7..14ee5cad 100644 --- a/src/libostree/ostree-sysroot-private.h +++ b/src/libostree/ostree-sysroot-private.h @@ -48,8 +48,7 @@ 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 03e742f1..a3e9e75d 100644 --- a/src/libostree/ostree-sysroot.c +++ b/src/libostree/ostree-sysroot.c @@ -135,8 +135,6 @@ 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); } @@ -815,26 +813,6 @@ 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 9639b4a5..3e3aeda9 100644 --- a/src/ostree/ot-remote-builtin-add.c +++ b/src/ostree/ot-remote-builtin-add.c @@ -31,7 +31,6 @@ 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" }, @@ -39,7 +38,6 @@ 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 } }; @@ -53,7 +51,6 @@ 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"); @@ -62,20 +59,6 @@ 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 7182e60b..cc06fe6f 100644 --- a/tests/admin-test.sh +++ b/tests/admin-test.sh @@ -232,19 +232,3 @@ 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 d7c1c6db..2b90c286 100755 --- a/tests/test-admin-deploy-grub2.sh +++ b/tests/test-admin-deploy-grub2.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..18" +echo "1..16" . $(dirname $0)/libtest.sh diff --git a/tests/test-admin-deploy-syslinux.sh b/tests/test-admin-deploy-syslinux.sh index 797836f0..70b3b4d3 100755 --- a/tests/test-admin-deploy-syslinux.sh +++ b/tests/test-admin-deploy-syslinux.sh @@ -19,7 +19,7 @@ set -euo pipefail -echo "1..18" +echo "1..16" . $(dirname $0)/libtest.sh diff --git a/tests/test-admin-deploy-uboot.sh b/tests/test-admin-deploy-uboot.sh index d9104f8c..d4c3a0db 100755 --- a/tests/test-admin-deploy-uboot.sh +++ b/tests/test-admin-deploy-uboot.sh @@ -20,7 +20,7 @@ set -euo pipefail -echo "1..19" +echo "1..17" . $(dirname $0)/libtest.sh