Revert "Add a notion of "physical" sysroot, use for remote writing"

This reverts commit 1eff3e8343. 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

@@ -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;

src/libostree/ostree-repo.c

@@ -32,7 +32,6 @@
 #include <glnx-console.h>
 
 #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))

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;

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;

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);

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"

tests/test-admin-deploy-grub2.sh

@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..18"
+echo "1..16"
 
 . $(dirname $0)/libtest.sh
 

tests/test-admin-deploy-syslinux.sh

@@ -19,7 +19,7 @@
 
 set -euo pipefail
 
-echo "1..18"
+echo "1..16"
 
 . $(dirname $0)/libtest.sh
 

tests/test-admin-deploy-uboot.sh

@@ -20,7 +20,7 @@
 
 set -euo pipefail
 
-echo "1..19"
+echo "1..17"
 
 . $(dirname $0)/libtest.sh