sysroot: Handle ro /boot but rw /sysroot

The recent change in https://github.com/coreos/fedora-coreos-config/pull/659
broke some of our tests that do `mount -o remount,rw /sysroot` but
leave `/boot` read-only.

We had code for having `/boot` read-only before `/sysroot` but
in practice we had a file descriptor for `/sysroot` that we opened
before the remount that would happen later on.

Clean things up here so that in the library, we also remount
`/boot` at the same time we remount `/sysroot` if either are readonly.

Delete the legacy code for remounting `/boot` rw if we're not in
a mount namespace.  I am fairly confident most users are either
using the `ostree` CLI, or they're using the mount namespace.

diff --git a/src/libostree/ostree-sysroot-deploy.c b/src/libostree/ostree-sysroot-deploy.c
index ad9b56d194ec792f692e96e1083b984b80b79969..0fff820be5be9548e4e9dbec9935bae406bbe240 100644 (file)
--- a/src/libostree/ostree-sysroot-deploy.c
+++ b/src/libostree/ostree-sysroot-deploy.c
@@ -56,9 +56,6 @@
 #define OSTREE_DEPLOYMENT_FINALIZING_ID SD_ID128_MAKE(e8,64,6c,d6,3d,ff,46,25,b7,79,09,a8,e7,a4,09,94)
 #endif
 
-static gboolean
-is_ro_mount (const char *path);
-
 /*
  * Like symlinkat() but overwrites (atomically) an existing
  * symlink.
@@ -2225,57 +2222,6 @@ cleanup_legacy_current_symlinks (OstreeSysroot         *self,
   return TRUE;
 }
 
-/* Detect whether or not @path refers to a read-only mountpoint. This is
- * currently just used to handle a potentially read-only /boot by transiently
- * remounting it read-write. In the future we might also do this for e.g.
- * /sysroot.
- */
-static gboolean
-is_ro_mount (const char *path)
-{
-#ifdef HAVE_LIBMOUNT
-  /* Dragging in all of this crud is apparently necessary just to determine
-   * whether something is a mount point.
-   *
-   * Systemd has a totally different implementation in
-   * src/basic/mount-util.c.
-   */
-  struct libmnt_table *tb = mnt_new_table_from_file ("/proc/self/mountinfo");
-  struct libmnt_fs *fs;
-  struct libmnt_cache *cache;
-  gboolean is_mount = FALSE;
-  struct statvfs stvfsbuf;
-
-  if (!tb)
-    return FALSE;
-
-  /* to canonicalize all necessary paths */
-  cache = mnt_new_cache ();
-  mnt_table_set_cache (tb, cache);
-
-  fs = mnt_table_find_target(tb, path, MNT_ITER_BACKWARD);
-  is_mount = fs && mnt_fs_get_target (fs);
-#ifdef HAVE_MNT_UNREF_CACHE
-  mnt_unref_table (tb);
-  mnt_unref_cache (cache);
-#else
-  mnt_free_table (tb);
-  mnt_free_cache (cache);
-#endif
-
-  if (!is_mount)
-    return FALSE;
-
-  /* We *could* parse the options, but it seems more reliable to
-   * introspect the actual mount at runtime.
-   */
-  if (statvfs (path, &stvfsbuf) == 0)
-    return (stvfsbuf.f_flag & ST_RDONLY) != 0;
-
-#endif
-  return FALSE;
-}
-
 /**
  * ostree_sysroot_write_deployments:
  * @self: Sysroot
@@ -2577,42 +2523,13 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
     }
   else
     {
-      gboolean boot_was_ro_mount = FALSE;
-      if (self->booted_deployment)
-        boot_was_ro_mount = is_ro_mount ("/boot");
-
-      g_debug ("boot is ro: %s", boot_was_ro_mount ? "yes" : "no");
-
-      if (boot_was_ro_mount)
-        {
-          if (mount ("/boot", "/boot", NULL, MS_REMOUNT | MS_SILENT, NULL) < 0)
-            return glnx_throw_errno_prefix (error, "Remounting /boot read-write");
-        }
-
       if (!_ostree_sysroot_query_bootloader (self, &bootloader, cancellable, error))
         return FALSE;
 
       bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader);
 
-      /* Note equivalent of try/finally here */
-      gboolean success = write_deployments_bootswap (self, new_deployments, opts, bootloader,
-                                                     &syncstats, cancellable, error);
-      /* Below here don't set GError until the if (!success) check.
-       * Note we only bother remounting if a mount namespace isn't in use.
-       * */
-      if (boot_was_ro_mount && !self->mount_namespace_in_use)
-        {
-          if (mount ("/boot", "/boot", NULL, MS_REMOUNT | MS_RDONLY | MS_SILENT, NULL) < 0)
-            {
-              /* Only make this a warning because we don't want to
-               * completely bomb out if some other process happened to
-               * jump in and open a file there.  See above TODO
-               * around doing this in a new mount namespace.
-               */
-              g_printerr ("warning: Failed to remount /boot read-only: %s\n", strerror (errno));
-            }
-        }
-      if (!success)
+      if (!write_deployments_bootswap (self, new_deployments, opts, bootloader,
+                                       &syncstats, cancellable, error))
         return FALSE;
     }
 

diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c
index d04b665db9bb7c97c8e20946d0bcdbf246a4cd4e..45baf8839d2284fa63bb3933eea27c401e1f4ba4 100644 (file)
--- a/src/libostree/ostree-sysroot.c
+++ b/src/libostree/ostree-sysroot.c
@@ -295,6 +295,31 @@ _ostree_sysroot_ensure_boot_fd (OstreeSysroot *self, GError **error)
   return TRUE;
 }
 
+static gboolean
+remount_writable (const char *path, gboolean *did_remount, GError **error)
+{
+  *did_remount = FALSE;
+  struct statvfs stvfsbuf;
+  if (statvfs (path, &stvfsbuf) < 0)
+    {
+      if (errno != ENOENT)
+        return glnx_throw_errno_prefix (error, "statvfs(%s)", path);
+      else
+        return TRUE;
+    }
+
+  if ((stvfsbuf.f_flag & ST_RDONLY) != 0)
+    {
+      /* OK, let's remount writable. */
+      if (mount (path, path, NULL, MS_REMOUNT | MS_RELATIME, "") < 0)
+        return glnx_throw_errno_prefix (error, "Remounting %s read-write", path);
+      *did_remount = TRUE;
+      g_debug ("remounted %s writable", path);
+    }
+
+  return TRUE;
+}
+
 /* Remount /sysroot read-write if necessary */
 gboolean
 _ostree_sysroot_ensure_writable (OstreeSysroot      *self,
@@ -314,19 +339,19 @@ _ostree_sysroot_ensure_writable (OstreeSysroot      *self,
   if (!self->root_is_ostree_booted)
     return TRUE;
 
-  /* Check if /sysroot is a read-only mountpoint */
-  struct statvfs stvfsbuf;
-  if (statvfs ("/sysroot", &stvfsbuf) < 0)
-    return glnx_throw_errno_prefix (error, "fstatvfs(/sysroot)");
-  if ((stvfsbuf.f_flag & ST_RDONLY) == 0)
-    return TRUE;
+  /* In these cases we also require /boot */
+  if (!_ostree_sysroot_ensure_boot_fd (self, error))
+    return FALSE;
 
-  /* OK, let's remount writable. */
-  if (mount ("/sysroot", "/sysroot", NULL, MS_REMOUNT | MS_RELATIME, "") < 0)
-    return glnx_throw_errno_prefix (error, "Remounting /sysroot read-write");
+  gboolean did_remount_sysroot = FALSE;
+  if (!remount_writable ("/sysroot", &did_remount_sysroot, error))
+    return FALSE;
+  gboolean did_remount_boot = FALSE;
+  if (!remount_writable ("/boot", &did_remount_boot, error))
+    return FALSE;
 
-  /* Reopen our fd */
-  glnx_close_fd (&self->sysroot_fd);
+  /* Now close and reopen our file descriptors */
+  ostree_sysroot_unload (self);
   if (!ensure_sysroot_fd (self, error))
     return FALSE;
 

diff --git a/src/libotutil/otutil.h b/src/libotutil/otutil.h
index 7db7270da0e16204a22d748f7a23dcba6b6ad24f..4cc5cd9f178759fc07cb51fd6fcd6684fd4297b8 100644 (file)
--- a/src/libotutil/otutil.h
+++ b/src/libotutil/otutil.h
@@ -34,6 +34,12 @@
 #define OT_VARIANT_BUILDER_INITIALIZER {{{0,}}}
 #endif
 
+static inline const char *
+ot_booltostr (int b)
+{
+  return b ? "true" : "false";
+}
+
 #define ot_gobject_refz(o) (o ? g_object_ref (o) : o)
 
 #define ot_transfer_out_value(outp, srcp) G_STMT_START {   \