Allow passing absolute paths to initrd and vmlinuz #121

Johan-Liebert1 · 2025-05-09T10:55:48Z

Grub needs absolute paths to initrd and vmlinuz if we do not have /boot in a boot partition, which we do not in bootc.

cgwalters · 2025-05-09T14:09:39Z

if we do not have /boot in a boot partition, which we do not in bootc.

Not by default, but we definitely can to be clear.

cgwalters

Per previous debate overall I just really do not like this idea of mutating bootloader entries shipped in the image and would prefer that we generate them client side. This logic would need to fundamentally change for that (and look more similar to what kernel-install and ostree to today).

But from my PoV that isn't blocking experimentation.

cgwalters · 2025-05-09T14:15:06Z

src/bootloader.rs

    // the file into the given 'entry_id' pathname and rename the entry file itself to
    // "{entry_id}.conf".
-    pub fn relocate(&mut self, entry_id: &str) {
+    pub fn relocate(&mut self, entry_id: &str, abs_entry_path: Option<&str>) -> Result<()> {


I think it'd be more consistent if this was Option<&Path>

Yeah, std::Path would've been a better choice here. Unsure why we went with &str, maybe because of Rust shenanigans? @allisonkarlitskaya

It's because it's a string in the boot loader file, which is the thing that causes us to go fetch these files in the first place. It's not possible for us to have non-utf8 boot resources...

resisting camino urges intensifies

cgwalters · 2025-05-09T14:16:34Z

src/bootloader.rs

                let new = format!("/{entry_id}/{basename}");
+                let new_entry_path = Path::new(&new);


Except yeah, above we have a string and so...this is where camino makes sense. It's honestly also one of the reasons I just ended up strictly denying non-UTF8 filenames in ostree, dealing with it is just a giant pain and there's so many file formats which reference filenames that just don't support non-UTF8 filenames, or would be extremely awkward to do.

allisonkarlitskaya

The first patch should absolutely just go in as-is. That's clearly an oversight on my part. We might even add the relocation step to the write_type1 function to save a bit of repetition (which is what ultimately led to the bug).

For the second one, I think the issue here is that it's not clear what "bootdir" refers to. Up to now (in my brain, at least) bootdir was one thing: the root of the boot partition. Now we have the possibility of not having a separate boot partition but rather /boot/ inside of another partition. I think this is a violation of the bootloader spec, but let's set that aside.

How about we rather do something like having two parameters:

an absolute path to the partition which contains the boot stuff
an optional relative path to the boot directory within that partition

So for a bootc system this would look like "/sysroot" and Some("boot") respectively. For the examples it would look like "/boot" and None.

That would allow us to:

properly locate our loader/entries/ directory
properly place our boot resources inside of a subdir
properly refer to those boot resources in our loader entry file relative to the root of the partition
not have any redundancy in the path components we provide

allisonkarlitskaya · 2025-05-12T10:58:42Z

src/bootloader.rs

    // the file into the given 'entry_id' pathname and rename the entry file itself to
    // "{entry_id}.conf".
-    pub fn relocate(&mut self, entry_id: &str) {
+    pub fn relocate(&mut self, entry_id: &str, abs_entry_path: Option<&str>) -> Result<()> {


It's because it's a string in the boot loader file, which is the thing that causes us to go fetch these files in the first place. It's not possible for us to have non-utf8 boot resources...

resisting camino urges intensifies

allisonkarlitskaya · 2025-05-12T11:14:51Z

I think this is a violation of the bootloader spec

That sort of thing requires a citation, so here we go:

Note: In all cases the /loader/entries/ directory should be located directly in the root of the ESP or XBOOTLDR filesystem.

https://uapi-group.org/specifications/specs/boot_loader_specification/

cgwalters · 2025-05-12T13:39:35Z

I think this is a violation of the bootloader spec

Yes, the status quo with at least Fedora derivatives using grub and ostree (at least) is that it isn't set up matching the BLS. That's 100% something we'd like to fix (ostreedev/ostree#3359 is one notable ingredient).

allisonkarlitskaya

I'm very sorry. I thought I posted this review earlier today. It's the same as we discussed on matrix, though...

src/write_boot.rs

src/bootloader.rs

allisonkarlitskaya · 2025-05-13T12:56:15Z

src/write_boot.rs

+/// * entry_id       - In case of a BLS entry, the name of file to be generated in `loader/entries`
+/// * boot_subdir    - If `Some(path)`, the path is prepended to `initrd` and `linux` keys in the BLS entry
+///
+/// For example, if `boot_subdir = Some("/boot/1")` and `boot_partition = "/boot"`,


I think this is what I was trying to avoid.

I rather think that in this case we'd have:

boot_partition: /boot

boot_subdir: Some("1")

So in the BLS-compliant case where the boot directory is rooted in its own filesystem, we'd pass None for boot_subdir. We should think about this less in terms of mechanics like ("prepended to paths in the entry file") and more about what the user sees in terms of their config.

But again, we want absolute "boot" path. If the "boot" dir is mounted at /run/bootc/mounts/rootfs/boot we have no way of knowing what the "final" absolute path is going to be. It could be /rootfs/boot or it could just be /boot, and this is the crux of the issue.

I didn't like the idea of just assuming that the "final" directory in the boot_partition will be the actual one.

allisonkarlitskaya · 2025-05-19T17:33:58Z

crates/composefs-boot/src/write_boot.rs

-/// * entry_id       - In case of a BLS entry, the name of file to be generated in `loader/entries`
 /// * boot_subdir    - If `Some(path)`, the path is prepended to `initrd` and `linux` keys in the BLS entry
 ///
 /// For example, if `boot_subdir = Some("/boot/1")` and `boot_partition = "/boot"`,


The repetition of /boot is the thing that I'm objecting to. The path to the files contained in the entry should consist of concatenating boot_partition + boot_subdir + entry_id. I think we should talk about this in a call :)

Signed-off-by: Pragyan Poudyal <[email protected]>

allisonkarlitskaya

Looking better thanks, but I think with the args defined the way that they are defined now, we don't need to make things so complicated...

allisonkarlitskaya · 2025-05-20T11:45:09Z

crates/composefs-boot/src/bootloader.rs

    // the file into the given 'entry_id' pathname and rename the entry file itself to
    // "{entry_id}.conf".
-    pub fn relocate(&mut self, entry_id: &str) {
+    pub fn relocate(&mut self, entry_id: &str, boot_subdir: Option<&str>) -> Result<()> {


Same nags as before: try to keep these in the order that they get pasted back together... so when they appear, they are always in the order of: boot_partition, boot_subdir, entry_id.

Also: with the arguments defined as they are now, I don't think this function needs to be failable anymore...

allisonkarlitskaya · 2025-05-20T11:48:01Z

crates/composefs-boot/src/bootloader.rs

+
+                let final_path = if let Some(boot_subdir) = boot_subdir {
+                    Path::new(boot_subdir)
+                        .join(new_entry_path.strip_prefix("/").unwrap_or(new_entry_path))
+                } else {
+                    new_entry_path.to_path_buf()
+                };
+
+                let final_path = final_path
+                    .to_str()
+                    .ok_or_else(|| anyhow!("Failed to convert Path '{final_path:?}' to string"))?;
+
+                line.replace_range(range, final_path);


This seems really too complicated. Should just be something like:

let new = match boot_subdir { Some(subdir) => format!("/{subdir}/{entry_id}/{basename}"), None => format!("/{entry_id}/{basename}"), };

or so, no?

So, new here is where the loader file is created relative to the /boot directory and final is the final concatenated path in the loader/entries/<entry_id>

They have to be separate

The naming is what's weird here :)

ah sure. I failed to notice that we use new for both things. But even still, you could let new_entry_path = match ... as per the previous comment, no?

allisonkarlitskaya · 2025-05-20T11:49:32Z

crates/composefs-boot/src/write_boot.rs

-    bootdir: &Path,
+    boot_partition: &Path,
    entry_id: Option<&str>,
+    boot_subdir: Option<&str>,


Order again: boot_partition, boot_subdir, entry_id.

This is correct, no?

Yes. I think I wrote that to a previous iteration.

allisonkarlitskaya · 2025-05-20T11:50:59Z

crates/composefs-boot/src/write_boot.rs

+                t1.relocate(name, boot_subdir)?;
            }
-            write_t1_simple(t1, bootdir, root_id, cmdline_extra, repo)?;
+            write_t1_simple(t1, boot_partition, root_id, cmdline_extra, repo)?;


Feels like that's gonna fail? boot_partition.join(boot_subdir), no? Or just pass both arguments through.

You might actually consider doing the relocation inside of the write code as well (it would reduce a small amount of duplication).

allisonkarlitskaya · 2025-05-20T11:54:51Z

It might also pay to get gibbity to write some tests for you...

allisonkarlitskaya · 2025-05-20T12:51:11Z

crates/composefs-boot/src/bootloader.rs

    // the file into the given 'entry_id' pathname and rename the entry file itself to
    // "{entry_id}.conf".
-    pub fn relocate(&mut self, entry_id: &str) {
+    pub fn relocate(&mut self, boot_subdir: Option<&str>, entry_id: &str) -> Result<()> {


Drop the Result<()> please

my bad, forgot to remove after refactor

allisonkarlitskaya · 2025-05-20T12:51:41Z

crates/composefs-boot/src/write_boot.rs

-    bootdir: &Path,
+    boot_partition: &Path,
    entry_id: Option<&str>,
+    boot_subdir: Option<&str>,


Yes. I think I wrote that to a previous iteration.

allisonkarlitskaya · 2025-05-20T12:52:42Z

crates/composefs-boot/src/bootloader.rs

+                let final_entry_path = if let Some(boot_subdir) = boot_subdir {
+                    format!("/{boot_subdir}{new}")
+                } else {
+                    new.clone()


I guess you could just move (and duplicate) the line.replace_range() into the if to avoid the clone if it bothers you ;)

duplication bothers me a bit more than a String clone :)

Grub needs absolute paths to initrd and vmlinuz if we do not have `/boot` in a boot partition, which we do not in bootc. Add param `boot_subdir` which acts like a subdirectory in the boot directory in case the boot partition is mounted in another directory. Signed-off-by: Pragyan Poudyal <[email protected]>

allisonkarlitskaya

Thanks for all the iterations. Looks good now!

Johan-Liebert1 requested a review from allisonkarlitskaya May 9, 2025 10:55

Johan-Liebert1 force-pushed the abs-boot-path branch from 8d44bd2 to 7115f34 Compare May 9, 2025 10:58

cgwalters approved these changes May 9, 2025

View reviewed changes

allisonkarlitskaya reviewed May 12, 2025

View reviewed changes

Johan-Liebert1 force-pushed the abs-boot-path branch from 7115f34 to 57e3a8d Compare May 13, 2025 06:38

Johan-Liebert1 requested a review from allisonkarlitskaya May 13, 2025 06:49

allisonkarlitskaya requested changes May 13, 2025

View reviewed changes

Johan-Liebert1 force-pushed the abs-boot-path branch from b92ace4 to ea4d3a7 Compare May 16, 2025 04:21

allisonkarlitskaya reviewed May 19, 2025

View reviewed changes

Add boot relocation for BootEntry::UsrLibModulesVmLinuz

4572186

Signed-off-by: Pragyan Poudyal <[email protected]>

allisonkarlitskaya reviewed May 20, 2025

View reviewed changes

Johan-Liebert1 force-pushed the abs-boot-path branch 2 times, most recently from 2cc49ea to 49ab4c8 Compare May 20, 2025 12:34

allisonkarlitskaya reviewed May 20, 2025

View reviewed changes

Johan-Liebert1 force-pushed the abs-boot-path branch from 49ab4c8 to 47e5df8 Compare May 20, 2025 14:40

Johan-Liebert1 force-pushed the abs-boot-path branch from 47e5df8 to f8f9323 Compare May 20, 2025 14:42

allisonkarlitskaya approved these changes May 20, 2025

View reviewed changes

allisonkarlitskaya merged commit 1f7064d into containers:main May 20, 2025
12 checks passed

		let new = format!("/{entry_id}/{basename}");
		let new_entry_path = Path::new(&new);

Allow passing absolute paths to initrd and vmlinuz #121

Allow passing absolute paths to initrd and vmlinuz #121

Uh oh!

Conversation

Johan-Liebert1 commented May 9, 2025

Uh oh!

cgwalters commented May 9, 2025

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonkarlitskaya commented May 12, 2025

Uh oh!

cgwalters commented May 12, 2025

Uh oh!

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonkarlitskaya commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees