generator: Fixes for Android Boot environment #3192

ericcurtin · 2024-02-21T16:18:10Z

In Android Boot environment we do not parse ostree= karg to determine
what directory to boot into, alternatively we do this based on the
androidboot.slot_suffix= karg. But we do set ostree=true karg to denote
that we are indeed booting an ostree environment (required for some
systemd unit files). This change accounts for this approach in the
systemd generator. In this case androidboot.slot_suffix= points you to
/ostree/root.[a|b] and then that points you to the directory to boot
into in /ostree/deploy... Here is what a cmdline may look like in this
type of environment:

androidboot.slot_suffix=_a androidboot.bootdevice=*.ufshc root=PARTLABEL=system_a root=UUID=76a22bf4-f153-4541-b6c7-0332c0dfaeac rw ostree=true loglevel=4 acpi=off console=ttyAMA0 systemd.show_status=auto libahci.ignore_sss=1 slub_debug=FPZ fsck.mode=skip rcupdate.rcu_normal_after_boot=0 rcupdate.rcu_expedited=1

ericcurtin · 2024-02-21T16:19:05Z

Will "undraft" this PR pending testing...

ericcurtin · 2024-02-21T16:57:51Z

Re-pushing something shortly hold tight.

In Android Boot environment we do not parse ostree= karg to determine what directory to boot into, alternatively we do this based on the androidboot.slot_suffix= karg. But we do set ostree=true karg to denote that we are indeed booting an ostree environment (required for some systemd unit files). This change accounts for this approach in the systemd generator. In this case androidboot.slot_suffix= points you to /ostree/root.[a|b] and then that points you to the directory to boot into in /ostree/deploy... Here is what a cmdline may look like in this type of environment: androidboot.slot_suffix=_a androidboot.bootdevice=*.ufshc root=PARTLABEL=system_a root=UUID=76a22bf4-f153-4541-b6c7-0332c0dfaeac rw ostree=true loglevel=4 acpi=off console=ttyAMA0 systemd.show_status=auto libahci.ignore_sss=1 slub_debug=FPZ fsck.mode=skip rcupdate.rcu_normal_after_boot=0 rcupdate.rcu_expedited=1 Signed-off-by: Eric Curtin <[email protected]>

ericcurtin · 2024-02-21T22:03:32Z

/ok-to-test

ericcurtin · 2024-02-22T10:56:16Z

src/libostree/ostree-sysroot.c

+                       "ostree=/ostree/boot.BOOTVERSION/OSNAME/BOOTCSUM/TREESERIAL or aboot method",
+                       to_parse);
+
+  if (is_aboot)


This is the main difference of this PR, the systemd generator needs to parse the OSNAME from the directory structure which is a little different in the aboot case.

jmarrero

lgtm

jlebon · 2024-02-22T16:05:48Z

src/libostree/ostree-impl-system-generator.c

@@ -261,17 +262,19 @@ _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_autofree char *ostree_cmdline = otcore_find_proc_cmdline_key (cmdline, "ostree");
-
+  g_autoptr (GError) otcore_get_ostree_target_error = NULL;


This isn't being checked. But anyway, why not pass error to the function? Looking at the code, not finding an ostree= karg isn't fatal.

We don't actually care so much if otcore_get_ostree_target fails but if there is no karg found for ostree selection we should return true for the CoreOS live environments.

If the androidboot karg doesn't exist all behavior is identical to what this code was like previous to this commit...

There is a "error" passed to this function, I deliberately didn't use it and used this one instead otcore_get_ostree_target_error, because we don't care so much about the otcore_get_ostree_target_error errors. But we do care about the "error" errors.

Or are we saying:

if (!otcore_get_ostree_target (cmdline, &is_aboot, &ostree_target, NULL)

should work? Didn't do this just in case of NULL ptr dereference or something, but if you are confident this works ok I trust you 😄

It's totally OK to pass NULL for the GError**. But I guess the question is, do we want to ignore the "androidboot.slot_suffix invalid: %s" error? I don't see why we would special-case that one. We're strict everywhere else in this function.

But actually, I think the issue is that the logic here is not correct. It should be e.g.

if (!otcore_get_ostree_target (cmdline, &is_aboot, &ostree_target, error)) return FALSE; /* This could happen ... */ if (!ostree_target) return TRUE;

jlebon · 2024-02-22T16:52:59Z

src/libotcore/otcore-prepare-root.c

@@ -84,8 +84,10 @@ otcore_get_ostree_target (const char *cmdline, char **out_target, GError **error

  // First, handle the Android boot case
  g_autofree char *slot_suffix = otcore_find_proc_cmdline_key (cmdline, "androidboot.slot_suffix");
+  *is_aboot = false;


Looking at the callers, might as well make this argument optional so they can just pass NULL if they're not interested.

Are there examples of optional arguments in this codebase? I know we can't do that in C generally but maybe there's some extension to C etc. that does this we are using that I'm not aware of.

In this context, optional means that the caller is allowed to pass NULL. So then in this function we'd do e.g.

if (is_aboot) *is_aboot = ...

jlebon · 2024-02-22T16:59:58Z

src/libostree/ostree-impl-system-generator.c

  /* 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. */


I think now that the conditional is more complex, let's tweak the comment slightly to make it clearer that we're referring to the "no ostree= karg exists". E.g.

Suggested change

/* 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 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. */

I think you've identified generally this comment no longer makes a lot of sense, the only place the function fails is in the case where slot_suffix isn't right

I think this relates to https://github.com/ostreedev/ostree/pull/3192/files/4a71845b127f8198c66d2fd2f024db8b01afb618#r1499795034

I'll rewrite the comment in this area, it's gonna be tomorrow morning, been online waaay too many hours this week

jlebon · 2024-02-22T17:01:10Z

src/libostree/ostree-sysroot-private.h

@@ -19,6 +19,8 @@

 #pragma once

+#include <stdbool.h>


Minor: we do use bool in a few places already but not really in function signatures. Let's just keep using gboolean for consistency?

jlebon · 2024-02-22T17:16:26Z

src/libostree/ostree-sysroot.c

+      regex = g_regex_new (is_aboot ? "^deploy/([^/]+)/"
+                                    : "^/ostree/boot.([01])/([^/]+)/([^/]+)/([0-9]+)$",
+                           0, 0, NULL);


Having a conditional selector for the regex in a g_once looks highly suspect here. I think in practice it doesn't matter too much because you probably only have all aboot deployments or none. But the way the API for this function is structured leaves the door open. ISTM we probably want two static GRegexs instead.

The thing is I'd end up doing:

if (!g_regex_match (is_aboot ? aboot_regex : non_aboot_regex, to_parse, 0, &match))

so it wouldn't make a whole pile of difference.

/proc/cmdline is an immutable file. Each process re-runs g_once_init_enter.

We can do code like above (technically they are different regex strings), but it's not really gonna change much in reality. It's also gonna be more lines of duplicate code.

I'll just write a _ostree_sysroot_parse_bootlink_aboot function altogether

Not that much duplicate code. Basically I'm saying something like:

static GRegex *regex_aboot; static GRegex *regex_ostree; if (g_once_init_enter (...)) { regex_aboot = ... regex_ostree = ... }

and move the ternary down to g_regex_match.

The difference is that the function correctly handles is_aboot actually being different on multiple calls in the same invocation. If we're saying that's never gonna happen, then I would refactor things to lift is_aboot to e.g. the OstreeSysroot level or so to say "this is an aboot-managed machine". Short-term, just handling this here correctly makes the code more obviously right when reading the function.

I wrote a _ostree_sysroot_parse_bootlink_aboot function instead in #3195, the only thing the aboot version had in common was that it was using a regex, it was starting to get messey.

jmarrero · 2024-02-22T17:27:41Z

@ericcurtin can you address the @jlebon comments before I do the release?

ericcurtin · 2024-02-22T17:50:45Z

I'll open another PR

ericcurtin self-assigned this Feb 21, 2024

ericcurtin requested review from alexlarsson and jmarrero February 21, 2024 16:18

ericcurtin marked this pull request as draft February 21, 2024 16:18

openshift-ci bot added the do-not-merge/work-in-progress label Feb 21, 2024

ericcurtin force-pushed the ostree-impl-system-generator-aboot branch from fadec22 to 81b94ab Compare February 21, 2024 18:55

github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Feb 21, 2024

ericcurtin force-pushed the ostree-impl-system-generator-aboot branch 2 times, most recently from c3813e3 to a8f8ca8 Compare February 21, 2024 19:59

ericcurtin force-pushed the ostree-impl-system-generator-aboot branch from a8f8ca8 to 4a71845 Compare February 21, 2024 20:55

ericcurtin marked this pull request as ready for review February 21, 2024 20:56

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 21, 2024

openshift-ci bot added the ok-to-test label Feb 21, 2024

ericcurtin commented Feb 22, 2024

View reviewed changes

ericcurtin requested review from cgwalters and jlebon February 22, 2024 15:38

jmarrero approved these changes Feb 22, 2024

View reviewed changes

ericcurtin merged commit e790e79 into ostreedev:main Feb 22, 2024
24 checks passed

ericcurtin deleted the ostree-impl-system-generator-aboot branch February 22, 2024 17:06

jlebon reviewed Feb 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generator: Fixes for Android Boot environment #3192

generator: Fixes for Android Boot environment #3192

ericcurtin commented Feb 21, 2024 •

edited

Loading

ericcurtin commented Feb 21, 2024

ericcurtin commented Feb 21, 2024

ericcurtin commented Feb 21, 2024

ericcurtin Feb 22, 2024

jmarrero left a comment

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024

ericcurtin Feb 22, 2024

ericcurtin Feb 22, 2024

ericcurtin Feb 22, 2024

jlebon Feb 22, 2024

jlebon Feb 22, 2024

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024

jlebon Feb 22, 2024

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024

ericcurtin Feb 22, 2024

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024 •

edited

Loading

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024 •

edited

Loading

ericcurtin Feb 22, 2024

jlebon Feb 22, 2024

ericcurtin Feb 22, 2024

jmarrero commented Feb 22, 2024

ericcurtin commented Feb 22, 2024

generator: Fixes for Android Boot environment #3192

generator: Fixes for Android Boot environment #3192

Conversation

ericcurtin commented Feb 21, 2024 • edited Loading

ericcurtin commented Feb 21, 2024

ericcurtin commented Feb 21, 2024

ericcurtin commented Feb 21, 2024

Choose a reason for hiding this comment

jmarrero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericcurtin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericcurtin Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarrero commented Feb 22, 2024

ericcurtin commented Feb 22, 2024

ericcurtin commented Feb 21, 2024 •

edited

Loading

ericcurtin Feb 22, 2024 •

edited

Loading

ericcurtin Feb 22, 2024 •

edited

Loading