bootloader: fold all Android Bootloader specific logic into prepare-root

[ericcurtin]

Now that we use androidboot.slot_suffix karg to determine whether we
boot into /ostree/root.a or /ostree/root.b, we can use ostree= karg
simply for parsing the stateroot, although we will still boot into
what's pointed to by /ostree/root.a or /ostree/root.b.

[ericcurtin]

@cgwalters this is incomplete but just to give you an idea of where I'm going with this. When this is complete, we will either call _ostree_bootloader_aboot_parse_bootlink or the more generic parse_bootlink function depending on how we set up this interface.

[cgwalters]

Hmmmm. Maybe.

I was thinking a bit more like this:

diff --git a/src/libostree/ostree-sysroot-private.h b/src/libostree/ostree-sysroot-private.h
index 63a29418..da17e64b 100644
--- a/src/libostree/ostree-sysroot-private.h
+++ b/src/libostree/ostree-sysroot-private.h
@@ -80,6 +80,7 @@ struct OstreeSysroot
   GPtrArray *deployments;
   int bootversion;
   int subbootversion;
+  gboolean using_android_boot;
   OstreeDeployment *booted_deployment;
   OstreeDeployment *staged_deployment;
   GVariant *staged_deployment_data;

And then we just do different things depending on that. It's not totally clear to me that what we need to do for aboot will practically fit into the bootloader abstraction we have right now.

[cgwalters]

I think it will be very useful to us to clean up and centralize everywhere that we're parsing the kernel cmdline.

So even if using the bootloader flow was the right direction, I don't think adding another copy of the regex is right short term. Nothing would stop this code from using the existing _ostree_sysroot_parse_bootlink() internal API right?

[ericcurtin]

I think it will be very useful to us to clean up and centralize everywhere that we're parsing the kernel cmdline.

So even if using the bootloader flow was the right direction, I don't think adding another copy of the regex is right short term. Nothing would stop this code from using the existing _ostree_sysroot_parse_bootlink() internal API right?

The boolean is certainly simpler. What is the alternate path to parsing the directories differently in the Android Bootloader case?

[cgwalters]

I started on this

diff --git a/Makefile-otcore.am b/Makefile-otcore.am
index 8accf858..685a0d06 100644
--- a/Makefile-otcore.am
+++ b/Makefile-otcore.am
@@ -18,6 +18,7 @@ noinst_LTLIBRARIES += libotcore.la
 libotcore_la_SOURCES = \
 	src/libotcore/otcore.h \
     src/libotcore/otcore-ed25519-verify.c \
+	src/libotcore/otcore-boot.c \
 	$(NULL)
 
 libotcore_la_CFLAGS = $(AM_CFLAGS) -I$(srcdir)/libglnx -I$(srcdir)/src/libotutil -DLOCALEDIR=\"$(datadir)/locale\" $(OT_INTERNAL_GIO_UNIX_CFLAGS) $(OT_INTERNAL_GPGME_CFLAGS) $(OT_DEP_CRYPTO_LIBS) $(LIBSYSTEMD_CFLAGS)
diff --git a/src/libotcore/otcore.h b/src/libotcore/otcore.h
index 1b2973e5..f991d3c4 100644
--- a/src/libotcore/otcore.h
+++ b/src/libotcore/otcore.h
@@ -56,3 +56,17 @@ gboolean otcore_validate_ed25519_signature (GBytes *data, GBytes *pubkey, GBytes
 #define OTCORE_RUN_BOOTED_KEY_COMPOSEFS_SIGNATURE "composefs.signed"
 // This key will be present if the sysroot-ro flag was found
 #define OTCORE_RUN_BOOTED_KEY_SYSROOT_RO "sysroot-ro"
+
+typedef enum {
+    OTCORE_BOOT_TYPE_OSTREE,
+    OTCORE_BOOT_TYPE_ANDROID,
+} OtCoreBootType;
+
+typedef struct {
+  OtCoreBootType type;
+  char *ostree_link;
+  char android_boot_slot;  // If type == ANDROID, this will be either 'a' or 'b'
+} OtCoreBootEntry;
+
+OtCoreBootEntry*
+otcore_parse_commandline (const char *cmdline);
diff --git a/src/switchroot/ostree-mount-util.h b/src/switchroot/ostree-mount-util.h
index 85f3443e..9f102496 100644
--- a/src/switchroot/ostree-mount-util.h
+++ b/src/switchroot/ostree-mount-util.h
@@ -123,29 +123,6 @@ read_proc_cmdline_key (const char *key)
   return ret;
 }
 
-static inline char *
-get_aboot_root_slot (const char *slot_suffix)
-{
-  if (strcmp (slot_suffix, "_a") == 0)
-    return strdup ("/ostree/root.a");
-  else if (strcmp (slot_suffix, "_b") == 0)
-    return strdup ("/ostree/root.b");
-
-  errx (EXIT_FAILURE, "androidboot.slot_suffix invalid: %s", slot_suffix);
-
-  return NULL;
-}
-
-static inline char *
-get_ostree_target (void)
-{
-  autofree char *slot_suffix = read_proc_cmdline_key ("androidboot.slot_suffix");
-  if (slot_suffix)
-    return get_aboot_root_slot (slot_suffix);
-
-  return read_proc_cmdline_key ("ostree");
-}
-
 /* This is an API for other projects to determine whether or not the
  * currently running system is ostree-controlled.
  */
diff --git a/src/switchroot/ostree-prepare-root-static.c b/src/switchroot/ostree-prepare-root-static.c
index 06181c57..9a6a0fba 100644
--- a/src/switchroot/ostree-prepare-root-static.c
+++ b/src/switchroot/ostree-prepare-root-static.c
@@ -70,6 +70,82 @@
 
 #include "ostree-mount-util.h"
 
+static inline char *
+read_proc_cmdline (void)
+{
+  FILE *f = fopen ("/proc/cmdline", "r");
+  char *cmdline = NULL;
+  size_t len;
+
+  if (!f)
+    goto out;
+
+  /* Note that /proc/cmdline will not end in a newline, so getline
+   * will fail unelss we provide a length.
+   */
+  if (getline (&cmdline, &len, f) < 0)
+    goto out;
+  /* ... but the length will be the size of the malloc buffer, not
+   * strlen().  Fix that.
+   */
+  len = strlen (cmdline);
+
+  if (cmdline[len - 1] == '\n')
+    cmdline[len - 1] = '\0';
+out:
+  if (f)
+    fclose (f);
+  return cmdline;
+}
+
+static inline void
+cleanup_free (void *p)
+{
+  void **pp = (void **)p;
+  free (*pp);
+}
+
+static inline char *
+read_proc_cmdline_key (const char *key)
+{
+  char *cmdline = NULL;
+  const char *iter;
+  char *ret = NULL;
+  size_t key_len = strlen (key);
+
+  cmdline = read_proc_cmdline ();
+  if (!cmdline)
+    err (EXIT_FAILURE, "failed to read /proc/cmdline");
+
+  iter = cmdline;
+  while (iter != NULL)
+    {
+      const char *next = strchr (iter, ' ');
+      const char *next_nonspc = next;
+      while (next_nonspc && *next_nonspc == ' ')
+        next_nonspc += 1;
+      if (strncmp (iter, key, key_len) == 0 && iter[key_len] == '=')
+        {
+          const char *start = iter + key_len + 1;
+          if (next)
+            ret = strndup (start, next - start);
+          else
+            ret = strdup (start);
+          break;
+        }
+      iter = next_nonspc;
+    }
+
+  free (cmdline);
+  return ret;
+}
+
+static inline char *
+get_ostree_target (void)
+{
+  return read_proc_cmdline_key ("ostree");
+}
+
 static inline bool
 sysroot_is_configured_ro (const char *sysroot)
 {

basically:

• Let's fork the cmdline parsing for the -static.c path and not try to support aboot there
• Move the remaining cmdline parsing into the new otcore.la lib which is shared between ostree-prepare-root and libostree.so and will allow us to not duplicate code
• Probably we need to move the whole karg parsing infrastructure into otcore.la
• Then have both prepare-root and libostree-1.so use otcore.la for parsing a kernel cmdline

[ericcurtin]

Apologies, I don't 100% understand why we move the various functions into:

src/switchroot/ostree-prepare-root-static.c

we need them elsewhere right? If we want to avoid traversing any sort on aboot code in -static.c, we could just call "read_proc_cmdline_key ("ostree");" instead of the generic "get_ostree_target()". And call the ostree-only version of the function in the one other place it's likely required too.

Like for example the "read_proc_cmdline_key (const char *key)" function which is pretty generic which we use for "ostree", "ot-composefs" and "androidboot.slot_suffix", we move this function to src/switchroot/ostree-prepare-root-static.c in the above example. But that doesn't make sense right? We use it elsewhere, even if we remove the need for an ostree= karg, we would still need it for "ot-composefs" and "androidboot.slot_suffix" (and even if we redesigned composefs to store that somewhere else in the initrd, we would still need the function for "androidboot.slot_suffix" as it's out of our control).

[ericcurtin]

I did split "ostree-mount-util.h" into "ostree-mount-util-static.h" and ""ostree-mount-util.h" here though.

That way -static.c includes only includes static stuff, and the other files include everything...

[ericcurtin]

It's a pity this is the case "Unfortunately we can't easily use the libostree API to find the booted deployment since /boot might not have been mounted yet." ... Because if we could guarantee /boot was up, we could simply do a realpath and have the exact same parsing/regex approach for Android Bootloader and the normal approach.

But I have realised since #2937 we can use the same parsing technique for both, as although the ostree= karg is not so useful for finding the sysroot to boot into. We can still use it to parse the stateroot, because the stateroot doesn't really change.

[ericcurtin]

@cgwalters I have to test this, but this solves my ostree-system-generator problem and now the Android Bootloader specific code is just in one .c file.

[ericcurtin]

So I tested this, it unblocks me (now ostree= is not aboot):

# cat /proc/cmdline
androidboot.slot_suffix=_a androidboot.bootdevice=*.ufshc root=LABEL=root ro loglevel=4 efi=runtime libahci.ignore_sss=1 console=ttyAMA0 ostree=/ostree/boot.1/centos/a359d17f0e40f2b8e6c4ed3fd8d57ef5fa76239a3f8123c568d91f0b3ef6c548/0
# rpm-ostree install git && reboot
# cat /proc/cmdline
androidboot.slot_suffix=_b androidboot.bootdevice=*.ufshc root=LABEL=root ro loglevel=4 efi=runtime libahci.ignore_sss=1 console=ttyAMA0 ostree=/ostree/boot.1/centos/a359d17f0e40f2b8e6c4ed3fd8d57ef5fa76239a3f8123c568d91f0b3ef6c548/0

[cgwalters]

Thanks, this looks like a nice incremental improvement!

[cgwalters]

I suspect though that we may end up needing to care about aboot here too. But if you don't think so, then this is definitely better.

What I was looking at was pushing all the cmdline parsing deep into the generator code instead of this binary so it can more cleanly use the internals, but it is a bigger change.

[ericcurtin]

Yup we have to be mindful of it (it works in the code as of today, but I understand there are big changes coming so we revisiting this will occur at times), stateroot being somewhat frozen in the cmdline is ok (you can theoretically change it, you just got to deliver a new Android Boot Image bundle in your update).... But with the other fields that's less ok. But I'm happy to change things as this code evolves and changes.

[ericcurtin]

And I also agree we should try and do this kind of thing in future:

+typedef enum {
+    OTCORE_BOOT_TYPE_OSTREE,
+    OTCORE_BOOT_TYPE_ANDROID,
+} OtCoreBootType;
+
+typedef struct {
+  OtCoreBootType type;
+  char *ostree_link;
+  char android_boot_slot;  // If type == ANDROID, this will be either 'a' or 'b'
+} OtCoreBootEntry;

If the differences start to get larger than 10 lines of code. At the moment, since it's just a single if statement that's called once in the lifetime of a process, it's not worth it.... yet... at least

[cgwalters]

Inherently in an A/B setup we can't really handle stateroot (or at least not quite how it was designed). One option is to have a symlink /ostree/stateroot -> /ostree/deploy/$stateroot.

This also heavily relates to #2794 which I would definitely advocate doing in general for these use cases.

IOW the logic could be:

if (android boot karg exists) {
stateroot = std::fs::readlink(/ostree/stateroot).or_else("default")
} else {
stateroot = run_regex_on(proc_cmdline_value("ostree"))
}

or so

[ericcurtin]

Inherently in an A/B setup we can't really handle stateroot (or at least not quite how it was designed). One option is to have a symlink /ostree/stateroot -> /ostree/deploy/$stateroot.

This also heavily relates to #2794 which I would definitely advocate doing in general for these use cases.

IOW the logic could be:

if (android boot karg exists) { stateroot = std::fs::readlink(/ostree/stateroot).or_else("default") } else { stateroot = run_regex_on(proc_cmdline_value("ostree")) }

or so

This sounds good to me, nice and simple

[cgwalters]

Glancing at this osbuild has a lot of references to stateroot that would have to grow defaulting logic.