sysroot: Reword comment and use gboolean over bool, error handling

Be more explicit in the comment, and use gboolean over bool. Less header
inclusions when we use gboolean. Although bool is used in some places.
Write a separate _ostree_sysroot_parse_bootlink_aboot function for
aboot. Make is_aboot optional. Handle invalid androidboot karg and no
ostree and androidboot kargs differently.

Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
Signed-off-by: Eric Curtin <ecurtin@redhat.com>

diff --git a/src/libostree/ostree-impl-system-generator.c b/src/libostree/ostree-impl-system-generator.c
index bfda2b138f7a567947c5f6169274f3d3211f5bfa..c5fe503287436aa77ce7e30606d8b8557fa2dd35 100644 (file)
--- a/src/libostree/ostree-impl-system-generator.c
+++ b/src/libostree/ostree-impl-system-generator.c
@@ -126,6 +126,34 @@ require_internal_units (const char *normal_dir, const char *early_dir, const cha
 #endif
 }
 
+// Resolve symlink to return osname
+static gboolean
+_ostree_sysroot_parse_bootlink_aboot (const char *bootlink, char **out_osname, GError **error)
+{
+  static gsize regex_initialized;
+  static GRegex *regex;
+  g_autofree char *symlink_val = glnx_readlinkat_malloc (-1, bootlink, NULL, error);
+  if (!symlink_val)
+    return glnx_prefix_error (error, "Failed to read '%s' symlink", bootlink);
+
+  if (g_once_init_enter (&regex_initialized))
+    {
+      regex = g_regex_new ("^deploy/([^/]+)/", 0, 0, NULL);
+      g_assert (regex);
+      g_once_init_leave (&regex_initialized, 1);
+    }
+
+  g_autoptr (GMatchInfo) match = NULL;
+  if (!g_regex_match (regex, symlink_val, 0, &match))
+    return glnx_throw (error,
+                       "Invalid aboot symlink in /ostree, expected symlink to resolve to "
+                       "deploy/OSNAME/... instead it resolves to '%s'",
+                       symlink_val);
+
+  *out_osname = g_match_info_fetch (match, 1);
+  return TRUE;
+}
+
 /* Generate var.mount */
 static gboolean
 fstab_generator (const char *ostree_target, const bool is_aboot, const char *normal_dir,
@@ -144,8 +172,12 @@ fstab_generator (const char *ostree_target, const bool is_aboot, const char *nor
    * mounted yet.
    */
   g_autofree char *stateroot = NULL;
-  if (!_ostree_sysroot_parse_bootlink (ostree_target, is_aboot, NULL, &stateroot, NULL, NULL,
-                                       error))
+  if (is_aboot)
+    {
+      if (!_ostree_sysroot_parse_bootlink_aboot (ostree_target, &stateroot, error))
+        return glnx_prefix_error (error, "Parsing aboot stateroot");
+    }
+  else if (!_ostree_sysroot_parse_bootlink (ostree_target, NULL, &stateroot, NULL, NULL, error))
     return glnx_prefix_error (error, "Parsing stateroot");
 
   /* Load /etc/fstab if it exists, and look for a /var mount */
@@ -262,14 +294,16 @@ _ostree_impl_system_generator (const char *normal_dir, const char *early_dir, co
   if (!cmdline)
     return glnx_throw (error, "Failed to read /proc/cmdline");
 
-  g_autoptr (GError) otcore_get_ostree_target_error = NULL;
   g_autofree char *ostree_target = NULL;
-  bool is_aboot = false;
-  /* This could happen in CoreOS live environments, where we hackily mock
+  gboolean is_aboot = false;
+  if (!otcore_get_ostree_target (cmdline, &is_aboot, &ostree_target, error))
+    return glnx_prefix_error (error, "Invalid aboot ostree target");
+
+  /* If no `ostree=` karg exists, gracefully no-op.
+   * This could happen in CoreOS live environments, where we hackily mock
    * the `ostree=` karg for `ostree-prepare-root.service` specifically, but
    * otherwise that karg doesn't exist on the real command-line. */
-  if (!otcore_get_ostree_target (cmdline, &is_aboot, &ostree_target,
-                                 &otcore_get_ostree_target_error))
+  if (!ostree_target)
     return TRUE;
 
   if (!require_internal_units (normal_dir, early_dir, late_dir, error))

diff --git a/src/libostree/ostree-sysroot-private.h b/src/libostree/ostree-sysroot-private.h
index 0e9f15bd1609f1a2544503f1404c599b3621385c..297b3273a0df7f9cde7f57a13b4755ac6ace4e47 100644 (file)
--- a/src/libostree/ostree-sysroot-private.h
+++ b/src/libostree/ostree-sysroot-private.h
@@ -19,8 +19,6 @@
 
 #pragma once
 
-#include <stdbool.h>
-
 #include "libglnx.h"
 #include "ostree-bootloader.h"
 #include "ostree.h"
@@ -179,9 +177,8 @@ gboolean _ostree_sysroot_parse_bootdir_name (const char *name, char **out_osname
 gboolean _ostree_sysroot_list_all_boot_directories (OstreeSysroot *self, char ***out_bootdirs,
                                                     GCancellable *cancellable, GError **error);
 
-gboolean _ostree_sysroot_parse_bootlink (const char *bootlink, const bool is_aboot,
-                                         int *out_entry_bootversion, char **out_osname,
-                                         char **out_bootcsum, int *out_treebootserial,
-                                         GError **error);
+gboolean _ostree_sysroot_parse_bootlink (const char *bootlink, int *out_entry_bootversion,
+                                         char **out_osname, char **out_bootcsum,
+                                         int *out_treebootserial, GError **error);
 
 G_END_DECLS

diff --git a/src/libostree/ostree-sysroot.c b/src/libostree/ostree-sysroot.c
index 097feb8b2c7fd0c0185808705d119e4377040651..5b4617faa095aa866e62245939e4ecbce23b24d0 100644 (file)
--- a/src/libostree/ostree-sysroot.c
+++ b/src/libostree/ostree-sysroot.c
@@ -724,44 +724,24 @@ load_origin (OstreeSysroot *self, OstreeDeployment *deployment, GCancellable *ca
 
 // Parse the kernel argument ostree=
 gboolean
-_ostree_sysroot_parse_bootlink (const char *bootlink, const bool is_aboot,
-                                int *out_entry_bootversion, char **out_osname, char **out_bootcsum,
-                                int *out_treebootserial, GError **error)
+_ostree_sysroot_parse_bootlink (const char *bootlink, int *out_entry_bootversion, char **out_osname,
+                                char **out_bootcsum, int *out_treebootserial, GError **error)
 {
   static gsize regex_initialized;
   static GRegex *regex;
-  const char *to_parse = bootlink;
-  g_autofree char *symlink_val = NULL;
-  if (is_aboot)
-    {
-      symlink_val = glnx_readlinkat_malloc (-1, bootlink, NULL, error);
-      if (!symlink_val)
-        return glnx_throw (error, "Failed to read '%s' symlink", bootlink);
-
-      to_parse = symlink_val;
-    }
-
   if (g_once_init_enter (&regex_initialized))
     {
-      regex = g_regex_new (is_aboot ? "^deploy/([^/]+)/"
-                                    : "^/ostree/boot.([01])/([^/]+)/([^/]+)/([0-9]+)$",
-                           0, 0, NULL);
+      regex = g_regex_new ("^/ostree/boot.([01])/([^/]+)/([^/]+)/([0-9]+)$", 0, 0, NULL);
       g_assert (regex);
       g_once_init_leave (&regex_initialized, 1);
     }
 
   g_autoptr (GMatchInfo) match = NULL;
-  if (!g_regex_match (regex, to_parse, 0, &match))
+  if (!g_regex_match (regex, bootlink, 0, &match))
     return glnx_throw (error,
                        "Invalid ostree= argument '%s', expected "
-                       "ostree=/ostree/boot.BOOTVERSION/OSNAME/BOOTCSUM/TREESERIAL or aboot method",
-                       to_parse);
-
-  if (is_aboot)
-    {
-      *out_osname = g_match_info_fetch (match, 1);
-      return TRUE;
-    }
+                       "ostree=/ostree/boot.BOOTVERSION/OSNAME/BOOTCSUM/TREESERIAL",
+                       bootlink);
 
   g_autofree char *bootversion_str = g_match_info_fetch (match, 1);
   g_autofree char *treebootserial_str = g_match_info_fetch (match, 4);
@@ -798,7 +778,7 @@ parse_deployment (OstreeSysroot *self, const char *boot_link, OstreeDeployment *
 
   // Note is_boot should always be false here, this boot_link is taken from BLS file, not
   // /proc/cmdline, BLS files are present in aboot images
-  if (!_ostree_sysroot_parse_bootlink (boot_link, false, &entry_boot_version, &osname, &bootcsum,
+  if (!_ostree_sysroot_parse_bootlink (boot_link, &entry_boot_version, &osname, &bootcsum,
                                        &treebootserial, error))
     return FALSE;
 

diff --git a/src/libotcore/otcore-prepare-root.c b/src/libotcore/otcore-prepare-root.c
index 19f481c10e1edf3ab1a1e618d246536017b6aec2..03575a478c3aa975a3997c969a3e9a9b6b25de99 100644 (file)
--- a/src/libotcore/otcore-prepare-root.c
+++ b/src/libotcore/otcore-prepare-root.c
@@ -75,7 +75,8 @@ otcore_find_proc_cmdline_key (const char *cmdline, const char *key)
 //
 // If invalid data is found, @error will be set.
 gboolean
-otcore_get_ostree_target (const char *cmdline, bool *is_aboot, char **out_target, GError **error)
+otcore_get_ostree_target (const char *cmdline, gboolean *is_aboot, char **out_target,
+                          GError **error)
 {
   g_assert (cmdline);
   g_assert (out_target && *out_target == NULL);
@@ -84,10 +85,14 @@ otcore_get_ostree_target (const char *cmdline, bool *is_aboot, char **out_target
 
   // First, handle the Android boot case
   g_autofree char *slot_suffix = otcore_find_proc_cmdline_key (cmdline, "androidboot.slot_suffix");
-  *is_aboot = false;
+  if (is_aboot)
+    *is_aboot = false;
+
   if (slot_suffix)
     {
-      *is_aboot = true;
+      if (is_aboot)
+        *is_aboot = true;
+
       if (strcmp (slot_suffix, "_a") == 0)
         {
           *out_target = g_strdup (slot_a);

diff --git a/src/libotcore/otcore.h b/src/libotcore/otcore.h
index 79a10f1568647d4538fd71b80b44b9d20c755572..fc6b81ca1a0d58084e5c460bc29edce9b2a29803 100644 (file)
--- a/src/libotcore/otcore.h
+++ b/src/libotcore/otcore.h
@@ -44,7 +44,7 @@ gboolean otcore_validate_ed25519_signature (GBytes *data, GBytes *pubkey, GBytes
                                             bool *out_valid, GError **error);
 
 char *otcore_find_proc_cmdline_key (const char *cmdline, const char *key);
-gboolean otcore_get_ostree_target (const char *cmdline, bool *is_aboot, char **out_target,
+gboolean otcore_get_ostree_target (const char *cmdline, gboolean *is_aboot, char **out_target,
                                    GError **error);
 
 GKeyFile *otcore_load_config (int rootfs, const char *filename, GError **error);

diff --git a/src/switchroot/ostree-prepare-root.c b/src/switchroot/ostree-prepare-root.c
index dd973594f7e1fcc44ffd305e292ad1447261c077..b93e05c7b0fbc3ab5053c76776f25d453948b9e6 100644 (file)
--- a/src/switchroot/ostree-prepare-root.c
+++ b/src/switchroot/ostree-prepare-root.c
@@ -124,8 +124,7 @@ resolve_deploy_path (const char *root_mountpoint)
 
   g_autoptr (GError) error = NULL;
   g_autofree char *ostree_target = NULL;
-  bool is_aboot = false;
-  if (!otcore_get_ostree_target (kernel_cmdline, &is_aboot, &ostree_target, &error))
+  if (!otcore_get_ostree_target (kernel_cmdline, NULL, &ostree_target, &error))
     errx (EXIT_FAILURE, "Failed to determine ostree target: %s", error->message);
   if (!ostree_target)
     errx (EXIT_FAILURE, "No ostree target found");

diff --git a/tests/test-otcore.c b/tests/test-otcore.c
index bc2f5a95bc61b09c9c2a960c3f827d59900e3f46..2d8003e5af184de694f54ee60df1bebfdcfd3a87 100644 (file)
--- a/tests/test-otcore.c
+++ b/tests/test-otcore.c
@@ -36,47 +36,45 @@ test_prepare_root_cmdline (void)
 {
   g_autoptr (GError) error = NULL;
   g_autofree char *target = NULL;
-  bool is_aboot = false;
-
   static const char *notfound_cases[]
       = { "", "foo", "foo=bar baz  sometest", "xostree foo", "xostree=blah bar", NULL };
   for (const char **iter = notfound_cases; iter && *iter; iter++)
     {
       const char *tcase = *iter;
-      g_assert (otcore_get_ostree_target (tcase, &is_aboot, &target, &error));
+      g_assert (otcore_get_ostree_target (tcase, NULL, &target, &error));
       g_assert_no_error (error);
       g_assert (target == NULL);
     }
 
   // Test the default ostree=
-  g_assert (otcore_get_ostree_target ("blah baz=blah ostree=/foo/bar somearg", &is_aboot, &target,
-                                      &error));
+  g_assert (
+      otcore_get_ostree_target ("blah baz=blah ostree=/foo/bar somearg", NULL, &target, &error));
   g_assert_no_error (error);
   g_assert_cmpstr (target, ==, "/foo/bar");
   free (g_steal_pointer (&target));
 
   // Test android boot
-  g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_b somearg", &is_aboot,
+  g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_b somearg", NULL,
                                       &target, &error));
   g_assert_no_error (error);
   g_assert_cmpstr (target, ==, "/ostree/root.b");
   free (g_steal_pointer (&target));
 
-  g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_a somearg", &is_aboot,
+  g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_a somearg", NULL,
                                       &target, &error));
   g_assert_no_error (error);
   g_assert_cmpstr (target, ==, "/ostree/root.a");
   free (g_steal_pointer (&target));
 
   // And an expected failure to parse a "c" suffix
-  g_assert (!otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_c somearg",
-                                       &is_aboot, &target, &error));
+  g_assert (!otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_c somearg", NULL,
+                                       &target, &error));
   g_assert (error);
   g_assert (target == NULL);
   g_clear_error (&error);
 
   // And non-A/B androidboot
-  g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.somethingelse somearg", &is_aboot,
+  g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.somethingelse somearg", NULL,
                                       &target, &error));
   g_assert_no_error (error);
   g_assert_cmpstr (target, ==, "/ostree/root.a");