Merge pull request ostreedev#3195 from ericcurtin/pr3192_comments

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

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

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;
 

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

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

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

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