rust-vmm / linux-loader

Linux kernel loader
Apache License 2.0
181 stars 55 forks source link

Add support for empty init arguments in the command line parser #176

Open mschlumpp opened 7 months ago

mschlumpp commented 7 months ago

Summary of the PR

For automated scripts it is easier to keep the trailing -- separator than to separately check whether it is strictly necessary. Without this change the crate would include the trailing -- as a kernel argument. Downstream consumers such as firecracker then proceed to append further parameters to that string (mmio parameters). The final re-assembled string looked similar to this: <kernel params> -- <mmio-args>, therefore passing the mmio parameters not to the kernel but to the init part.

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

roypat commented 7 months ago

Hi @mschlumpp, Thank you for your PR! I think what you're trying to achieve is already partially supported by linux-loader (e.g. the first of the two test cases you add passes even without your changes). The only missing piece is that if the string passed to try_from has the form <boot args> -- (e.g. no trailing space after the --), then the -- will be considered part of the boot args instead of being stripped. For this, I think we can achieve what you want with a simpler change:

diff --git a/src/cmdline/mod.rs b/src/cmdline/mod.rs
index 2ac7c31..5687ca3 100644
--- a/src/cmdline/mod.rs
+++ b/src/cmdline/mod.rs
@@ -487,6 +487,10 @@ impl Cmdline {
             ),
         };

+        if boot_args.ends_with(INIT_ARGS_SEPARATOR.trim_end()) {
+            boot_args = &boot_args[..boot_args.len() - INIT_ARGS_SEPARATOR.trim().len()];
+        }
+
         boot_args = boot_args.trim();
         init_args = init_args.trim();

Given that we already trim the trailing -- if its followed by a space, that seems like a reasonable change to make, but I'd like to get some input from someone who knows more about the intricacies here (maybe @rbradford?).

Other than that, we should also adjust the documentation of try_from, to point out that it strips trailing --s. It should also get a changelog entry.