kargs: correctly split kernel command-line string with quoted arguments

[pcolberg]

Splitting the kernel command-line on whitespace breaks quoted arguments, e.g., "dyndbg="file drivers/base/firmware_loader/main.c +fmp"".

Use the bootc_kernel_cmdline crate to parse and split kernel arguments, which unbreaks deployment when a quoted kernel argument is present.

Closes: ostreedev/ostree#3544

[pcolberg]

While this resolves the issue, we may want to pass around the kernel command-line as bootc_kernel_cmdline::utf8::Cmdline and only convert to a Vec<String> right before calling into ostree. The fix could also use a test.

[jeckersb]

While this resolves the issue, we may want to pass around the kernel command-line as bootc_kernel_cmdline::utf8::Cmdline and only convert to a Vec<String> right before calling into ostree. The fix could also use a test.

Yeah there's a few uses of Vec::extend going on in bootc_kargs, when really what probably makes more sense is to use Cmdline::add_or_modify which will handle things a bit more gracefully, either appending the parameter if it's not already in the cmdline, or replacing the old value with the new value if the value already exists.

[cgwalters]

or replacing the old value with the new value if the value already exists.

It may not be a backcompact change but I'd probably say it should be an error to put something in the bootc kargs.d that overrides an already extant cmdline flag.

[pcolberg]

Please feel free to overwrite my branch any time, as it will take me a while to return to this.

[jeckersb]

Please feel free to overwrite my branch any time, as it will take me a while to return to this.

Working on it now, should have something soon.

[jeckersb]

or replacing the old value with the new value if the value already exists.

It may not be a backcompact change but I'd probably say it should be an error to put something in the bootc kargs.d that overrides an already extant cmdline flag.

👍 this will require tweaking kernel_cmdline a bit though, since right now add_or_modify only returns bool to indicate whether a modification happened or not. But it doesn't distinguish between a value being added vs an existing value being modified. I'll probably add a breaking change to make it return an enum instead to distinguish between Added, Modified, or Existed. Fortunately it doesn't appear we're using that API anywhere in composefs-rs so there's no cross-repo breakage.

[jeckersb]

It also occurs to me now that I have it close to working that the kernel_cmdline crate needs to be reworked a bit to allow duplicated parameters. For example, console=tty0 console=ttyS1 in the bootc_kargs tests is valid usage but the current implementation replaces instead of appending so we can't add both.

[cgwalters]

Hmmmm we really didn't support that? That seems like a big oversight

[jeckersb]

Hmmmm we really didn't support that? That seems like a big oversight

Well it would work if the data you fed into it already had the duplicate keys. So Cmdline::from("console=tty0 console=ttyS1").iter() would yield both console parameters. But the only way (currently) to add a parameter is via add_or_modify which discards any existing params with the same key.

Fortunately a simple fix, just need to add a new add method that will allow duplicate keys (but should be a no-op if the key already exists with the correct value).