Merge pull request #2224 from stb-tester/bootloader-refactorings

Bootloader probing and construction refactoring

src/libostree/ostree-repo-private.h

@@ -110,6 +110,28 @@ typedef enum {
   _OSTREE_FEATURE_YES,
 } _OstreeFeatureSupport;
 
+/* Possible values for the sysroot.bootloader configuration variable */
+typedef enum {
+  CFG_SYSROOT_BOOTLOADER_OPT_AUTO = 0,
+  CFG_SYSROOT_BOOTLOADER_OPT_NONE,
+  CFG_SYSROOT_BOOTLOADER_OPT_GRUB2,
+  CFG_SYSROOT_BOOTLOADER_OPT_SYSLINUX,
+  CFG_SYSROOT_BOOTLOADER_OPT_UBOOT,
+  CFG_SYSROOT_BOOTLOADER_OPT_ZIPL,
+  /* Non-exhaustive */
+} OstreeCfgSysrootBootloaderOpt;
+
+static const char* const CFG_SYSROOT_BOOTLOADER_OPTS_STR[] = {
+  /* This must be kept in the same order as the enum */
+  "auto",
+  "none",
+  "grub2",
+  "syslinux",
+  "uboot",
+  "zipl",
+  NULL,
+};
+
 /**
  * OstreeRepo:
  *
@@ -193,7 +215,7 @@ struct OstreeRepo {
   guint64 payload_link_threshold;
   gint fs_support_reflink; /* The underlying filesystem has support for ioctl (FICLONE..) */
   gchar **repo_finders;
-  gchar *bootloader; /* Configure which bootloader to use. */
+  OstreeCfgSysrootBootloaderOpt bootloader; /* Configure which bootloader to use. */
 
   OstreeRepo *parent_repo;
 };

src/libostree/ostree-repo.c

@@ -1048,7 +1048,6 @@ ostree_repo_finalize (GObject *object)
   g_mutex_clear (&self->txn_lock);
   g_free (self->collection_id);
   g_strfreev (self->repo_finders);
-  g_free (self->bootloader);
 
   g_clear_pointer (&self->remotes, g_hash_table_destroy);
   g_mutex_clear (&self->remotes_lock);
@@ -3186,28 +3185,28 @@ reload_sysroot_config (OstreeRepo          *self,
                        GCancellable        *cancellable,
                        GError             **error)
 {
-  { g_autofree char *bootloader = NULL;
+  g_autofree char *bootloader = NULL;
 
-    if (!ot_keyfile_get_value_with_default_group_optional (self->config, "sysroot",
-                                                           "bootloader", "auto",
-                                                           &bootloader, error))
-      return FALSE;
+  if (!ot_keyfile_get_value_with_default_group_optional (self->config, "sysroot",
+                                                         "bootloader", "auto",
+                                                         &bootloader, error))
+    return FALSE;
 
-    /* TODO: possibly later add support for specifying a generic bootloader
-     * binary "x" in /usr/lib/ostree/bootloaders/x). See:
-     * https://github.com/ostreedev/ostree/issues/1719
-     * https://github.com/ostreedev/ostree/issues/1801
-     * Also, dedup these strings with the bootloader implementations
-     */
-    if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none")
-          || g_str_equal (bootloader, "zipl")))
-      return glnx_throw (error, "Invalid bootloader configuration: '%s'", bootloader);
+  /* TODO: possibly later add support for specifying a generic bootloader
+   * binary "x" in /usr/lib/ostree/bootloaders/x). See:
+   * https://github.com/ostreedev/ostree/issues/1719
+   * https://github.com/ostreedev/ostree/issues/1801
+   */
+  for (int i = 0; CFG_SYSROOT_BOOTLOADER_OPTS_STR[i]; i++)
+    {
+      if (g_str_equal (bootloader, CFG_SYSROOT_BOOTLOADER_OPTS_STR[i]))
+        {
+          self->bootloader = (OstreeCfgSysrootBootloaderOpt) i;
+          return TRUE;
+        }
+    }
 
-    g_free (self->bootloader);
-    self->bootloader = g_steal_pointer (&bootloader);
-  }
-
-  return TRUE;
+  return glnx_throw (error, "Invalid bootloader configuration: '%s'", bootloader);
 }
 
 /**
@@ -6323,7 +6322,7 @@ ostree_repo_get_default_repo_finders (OstreeRepo *self)
  * Get the bootloader configured. See the documentation for the
  * "sysroot.bootloader" config key.
  *
- * Returns: bootloader configuration for the sysroot
+ * Returns: (transfer none): bootloader configuration for the sysroot
  * Since: 2019.2
  */
 const gchar *
@@ -6331,7 +6330,7 @@ ostree_repo_get_bootloader (OstreeRepo   *self)
 {
   g_return_val_if_fail (OSTREE_IS_REPO (self), NULL);
 
-  return self->bootloader;
+  return CFG_SYSROOT_BOOTLOADER_OPTS_STR[self->bootloader];
 }
 
 

src/libostree/ostree-sysroot-deploy.c

@@ -44,7 +44,6 @@
 #include "ostree-repo-private.h"
 #include "ostree-sysroot-private.h"
 #include "ostree-sepolicy-private.h"
-#include "ostree-bootloader-zipl.h"
 #include "ostree-deployment-private.h"
 #include "ostree-core-private.h"
 #include "ostree-linuxfsutil.h"
@@ -2561,7 +2560,6 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
   gboolean bootloader_is_atomic = FALSE;
   SyncStats syncstats = { 0, };
   g_autoptr(OstreeBootloader) bootloader = NULL;
-  const char *bootloader_config = NULL;
   if (!requires_new_bootversion)
     {
       if (!create_new_bootlinks (self, self->bootversion,
@@ -2593,29 +2591,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
             return glnx_throw_errno_prefix (error, "Remounting /boot read-write");
         }
 
-      OstreeRepo *repo = ostree_sysroot_repo (self);
-
-      bootloader_config = ostree_repo_get_bootloader (repo);
-
-      g_debug ("Using bootloader configuration: %s", bootloader_config);
-
-      if (g_str_equal (bootloader_config, "auto"))
-        {
-          if (!_ostree_sysroot_query_bootloader (self, &bootloader, cancellable, error))
-            return FALSE;
-        }
-      else if (g_str_equal (bootloader_config, "none"))
-        {
-          /* No bootloader specified; do not query bootloaders to run. */
-        }
-      else if (g_str_equal (bootloader_config, "zipl"))
-        {
-          /* Because we do not mark zipl as active by default, lets creating one here,
-           * which is basically the same what _ostree_sysroot_query_bootloader() does
-           * for other bootloaders if being activated.
-           * */
-          bootloader = (OstreeBootloader*) _ostree_bootloader_zipl_new (self);
-        }
+      if (!_ostree_sysroot_query_bootloader (self, &bootloader, cancellable, error))
+        return FALSE;
 
       bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader);
 
@@ -2646,6 +2623,7 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot     *self,
                        (bootloader_is_atomic ? "Transaction complete" : "Bootloader updated"),
                        requires_new_bootversion ? "yes" : "no",
                        new_deployments->len - self->deployments->len);
+    const gchar *bootloader_config = ostree_repo_get_bootloader (ostree_sysroot_repo (self));
     ot_journal_send ("MESSAGE_ID=" SD_ID128_FORMAT_STR, SD_ID128_FORMAT_VAL(OSTREE_DEPLOYMENT_COMPLETE_ID),
                      "MESSAGE=%s", msg,
                      "OSTREE_BOOTLOADER=%s", bootloader ? _ostree_bootloader_get_name (bootloader) : "none",

src/libostree/ostree-sysroot.c

@@ -36,6 +36,7 @@
 #include "ostree-bootloader-uboot.h"
 #include "ostree-bootloader-syslinux.h"
 #include "ostree-bootloader-grub2.h"
+#include "ostree-bootloader-zipl.h"
 
 /**
  * SECTION:ostree-sysroot
@@ -1324,6 +1325,34 @@ ostree_sysroot_repo (OstreeSysroot *self)
   return self->repo;
 }
 
+static OstreeBootloader*
+_ostree_sysroot_new_bootloader_by_type (
+    OstreeSysroot                 *sysroot,
+    OstreeCfgSysrootBootloaderOpt  bl_type)
+{
+  switch (bl_type)
+    {
+    case CFG_SYSROOT_BOOTLOADER_OPT_NONE:
+      /* No bootloader specified; do not query bootloaders to run. */
+      return NULL;
+    case CFG_SYSROOT_BOOTLOADER_OPT_GRUB2:
+      return (OstreeBootloader*) _ostree_bootloader_grub2_new (sysroot);
+    case CFG_SYSROOT_BOOTLOADER_OPT_SYSLINUX:
+      return (OstreeBootloader*) _ostree_bootloader_syslinux_new (sysroot);
+    case CFG_SYSROOT_BOOTLOADER_OPT_UBOOT:
+      return (OstreeBootloader*) _ostree_bootloader_uboot_new (sysroot);
+    case CFG_SYSROOT_BOOTLOADER_OPT_ZIPL:
+      /* We never consider zipl as active by default, so it can only be created
+       * if it's explicitly requested in the config */
+      return (OstreeBootloader*) _ostree_bootloader_zipl_new (sysroot);
+    case CFG_SYSROOT_BOOTLOADER_OPT_AUTO:
+      /* "auto" is handled by ostree_sysroot_query_bootloader so we should
+       * never get here: Fallthrough */
+    default:
+      g_assert_not_reached ();
+    }
+}
+
 /**
  * ostree_sysroot_query_bootloader:
  * @sysroot: Sysroot
@@ -1337,32 +1366,38 @@ _ostree_sysroot_query_bootloader (OstreeSysroot     *sysroot,
                                   GCancellable      *cancellable,
                                   GError           **error)
 {
-  gboolean is_active;
-  g_autoptr(OstreeBootloader) ret_loader =
-    (OstreeBootloader*)_ostree_bootloader_syslinux_new (sysroot);
-  if (!_ostree_bootloader_query (ret_loader, &is_active,
-                                 cancellable, error))
-    return FALSE;
+  OstreeRepo *repo = ostree_sysroot_repo (sysroot);
+  OstreeCfgSysrootBootloaderOpt bootloader_config = repo->bootloader;
 
-  if (!is_active)
-    {
-      g_object_unref (ret_loader);
-      ret_loader = (OstreeBootloader*)_ostree_bootloader_grub2_new (sysroot);
-      if (!_ostree_bootloader_query (ret_loader, &is_active,
-                                     cancellable, error))
-        return FALSE;
-    }
-  if (!is_active)
-    {
-      g_object_unref (ret_loader);
-      ret_loader = (OstreeBootloader*)_ostree_bootloader_uboot_new (sysroot);
-      if (!_ostree_bootloader_query (ret_loader, &is_active, cancellable, error))
-        return FALSE;
-    }
-  if (!is_active)
-    g_clear_object (&ret_loader);
+  g_debug ("Using bootloader configuration: %s",
+      CFG_SYSROOT_BOOTLOADER_OPTS_STR[bootloader_config]);
 
-  ot_transfer_out_value(out_bootloader, &ret_loader);
+  g_autoptr(OstreeBootloader) ret_loader = NULL;
+  if (bootloader_config == CFG_SYSROOT_BOOTLOADER_OPT_AUTO)
+    {
+      OstreeCfgSysrootBootloaderOpt probe[] = {
+        CFG_SYSROOT_BOOTLOADER_OPT_SYSLINUX,
+        CFG_SYSROOT_BOOTLOADER_OPT_GRUB2,
+        CFG_SYSROOT_BOOTLOADER_OPT_UBOOT,
+      };
+      for (int i = 0; i < G_N_ELEMENTS (probe); i++)
+        {
+          g_autoptr(OstreeBootloader) bl = _ostree_sysroot_new_bootloader_by_type (
+              sysroot, probe[i]);
+          gboolean is_active = FALSE;
+          if (!_ostree_bootloader_query (bl, &is_active, cancellable, error))
+            return FALSE;
+          if (is_active)
+            {
+              ret_loader = g_steal_pointer (&bl);
+              break;
+            }
+        }
+    }
+  else
+    ret_loader = _ostree_sysroot_new_bootloader_by_type (sysroot, bootloader_config);
+
+  ot_transfer_out_value (out_bootloader, &ret_loader)
   return TRUE;
 }