Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

generator: Fixes for Android Boot environment #3192

Merged
merged 1 commit into from
Feb 22, 2024
Merged

generator: Fixes for Android Boot environment #3192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/libostree/ostree-impl-system-generator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,8 @@ require_internal_units (const char *normal_dir, const char *early_dir, const cha

/* Generate var.mount */
static gboolean
fstab_generator (const char *ostree_cmdline, const char *normal_dir, const char *early_dir,
const char *late_dir, GError **error)
fstab_generator (const char *ostree_target, const bool is_aboot, const char *normal_dir,
const char *early_dir, const char *late_dir, GError **error)
{
#ifdef HAVE_LIBMOUNT
/* Not currently cancellable, but define a var in case we care later */
Expand All @@ -144,7 +144,8 @@ fstab_generator (const char *ostree_cmdline, const char *normal_dir, const char
* mounted yet.
*/
g_autofree char *stateroot = NULL;
if (!_ostree_sysroot_parse_bootlink (ostree_cmdline, NULL, &stateroot, NULL, NULL, error))
if (!_ostree_sysroot_parse_bootlink (ostree_target, is_aboot, NULL, &stateroot, NULL, NULL,
error))
return glnx_prefix_error (error, "Parsing stateroot");

/* Load /etc/fstab if it exists, and look for a /var mount */
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

g_autofree char *ostree_target = NULL;
bool is_aboot = false;
/* 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. */
Comment on lines 268 to 270
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@ericcurtin ericcurtin Feb 22, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!ostree_cmdline)
if (!otcore_get_ostree_target (cmdline, &is_aboot, &ostree_target,
&otcore_get_ostree_target_error))
return TRUE;

if (!require_internal_units (normal_dir, early_dir, late_dir, error))
return FALSE;
if (!fstab_generator (ostree_cmdline, normal_dir, early_dir, late_dir, error))
if (!fstab_generator (ostree_target, is_aboot, normal_dir, early_dir, late_dir, error))
return FALSE;

return TRUE;
Expand Down
9 changes: 6 additions & 3 deletions src/libostree/ostree-sysroot-private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

#pragma once

#include <stdbool.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM


#include "libglnx.h"
#include "ostree-bootloader.h"
#include "ostree.h"
Expand Down Expand Up @@ -177,8 +179,9 @@ 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, int *out_entry_bootversion,
char **out_osname, char **out_bootcsum,
int *out_treebootserial, 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);

G_END_DECLS
37 changes: 30 additions & 7 deletions src/libostree/ostree-sysroot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,24 +724,44 @@ load_origin (OstreeSysroot *self, OstreeDeployment *deployment, GCancellable *ca

// Parse the kernel argument ostree=
gboolean
_ostree_sysroot_parse_bootlink (const char *bootlink, int *out_entry_bootversion, char **out_osname,
char **out_bootcsum, int *out_treebootserial, GError **error)
_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)
{
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 ("^/ostree/boot.([01])/([^/]+)/([^/]+)/([0-9]+)$", 0, 0, NULL);
regex = g_regex_new (is_aboot ? "^deploy/([^/]+)/"
: "^/ostree/boot.([01])/([^/]+)/([^/]+)/([0-9]+)$",
0, 0, NULL);
Comment on lines +746 to +748
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@ericcurtin ericcurtin Feb 22, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just write a _ostree_sysroot_parse_bootlink_aboot function altogether

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

g_assert (regex);
g_once_init_leave (&regex_initialized, 1);
}

g_autoptr (GMatchInfo) match = NULL;
if (!g_regex_match (regex, bootlink, 0, &match))
if (!g_regex_match (regex, to_parse, 0, &match))
return glnx_throw (error,
"Invalid ostree= argument '%s', expected "
"ostree=/ostree/boot.BOOTVERSION/OSNAME/BOOTCSUM/TREESERIAL",
bootlink);
"ostree=/ostree/boot.BOOTVERSION/OSNAME/BOOTCSUM/TREESERIAL or aboot method",
to_parse);

if (is_aboot)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
*out_osname = g_match_info_fetch (match, 1);
return TRUE;
}

g_autofree char *bootversion_str = g_match_info_fetch (match, 1);
g_autofree char *treebootserial_str = g_match_info_fetch (match, 4);
Expand Down Expand Up @@ -775,7 +795,10 @@ parse_deployment (OstreeSysroot *self, const char *boot_link, OstreeDeployment *
g_autofree char *osname = NULL;
g_autofree char *bootcsum = NULL;
int treebootserial = -1;
if (!_ostree_sysroot_parse_bootlink (boot_link, &entry_boot_version, &osname, &bootcsum,

// 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,
&treebootserial, error))
return FALSE;

Expand Down
4 changes: 3 additions & 1 deletion src/libotcore/otcore-prepare-root.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ 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, char **out_target, GError **error)
otcore_get_ostree_target (const char *cmdline, bool *is_aboot, char **out_target, GError **error)
{
g_assert (cmdline);
g_assert (out_target && *out_target == NULL);
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = ...

if (slot_suffix)
{
*is_aboot = true;
if (strcmp (slot_suffix, "_a") == 0)
{
*out_target = g_strdup (slot_a);
Expand Down
3 changes: 2 additions & 1 deletion src/libotcore/otcore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ 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, char **out_target, GError **error);
gboolean otcore_get_ostree_target (const char *cmdline, bool *is_aboot, char **out_target,
GError **error);

GKeyFile *otcore_load_config (int rootfs, const char *filename, GError **error);

Expand Down
3 changes: 2 additions & 1 deletion src/switchroot/ostree-prepare-root.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ resolve_deploy_path (const char *root_mountpoint)

g_autoptr (GError) error = NULL;
g_autofree char *ostree_target = NULL;
if (!otcore_get_ostree_target (kernel_cmdline, &ostree_target, &error))
bool is_aboot = false;
if (!otcore_get_ostree_target (kernel_cmdline, &is_aboot, &ostree_target, &error))
errx (EXIT_FAILURE, "Failed to determine ostree target: %s", error->message);
if (!ostree_target)
errx (EXIT_FAILURE, "No ostree target found");
Expand Down
22 changes: 12 additions & 10 deletions tests/test-otcore.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,46 +36,48 @@ 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, &target, &error));
g_assert (otcore_get_ostree_target (tcase, &is_aboot, &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", &target, &error));
g_assert (otcore_get_ostree_target ("blah baz=blah ostree=/foo/bar somearg", &is_aboot, &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", &target,
&error));
g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_b somearg", &is_aboot,
&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", &target,
&error));
g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_a somearg", &is_aboot,
&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", &target,
&error));
g_assert (!otcore_get_ostree_target ("blah baz=blah androidboot.slot_suffix=_c somearg",
&is_aboot, &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", &target,
&error));
g_assert (otcore_get_ostree_target ("blah baz=blah androidboot.somethingelse somearg", &is_aboot,
&target, &error));
g_assert_no_error (error);
g_assert_cmpstr (target, ==, "/ostree/root.a");
free (g_steal_pointer (&target));
Expand Down