Switch bootc_kargs to use kernel_cmdline for parsing

[jeckersb]

Had this working earlier this afternoon but broke it in rebase somehow. Time for halloween festivities but wanted to throw it up incase my laptop is hit by a meteor over the weekend.

[jeckersb]

Rebased on #1729, so this is pending getting that sorted out and merged first.

[jeckersb]

Hooray for tests

verifying third boot
cmdline=[BOOT_IMAGE=(hd0,gpt3)/boot/ostree/default-7e0ca7b10ef8102e3e459b6a8de1b8d09f8c700e82e97d2af2c1d940d967c9ab/vmlinuz-6.17.6-200.fc42.x86_64, root=UUID=d83586c7-8a15-41d1-a383-aca4d4def032, rootflags=subvol=root, rw, boot=UUID=72a5cd34-0b37-4dc9-9907-0524161248c3, localtestkarg=somevalue, otherlocalkarg=42, kargsd-test=1, kargsd-othertest=2, console=ttyS0,115200n8, testing-kargsd=3, ostree=/ostree/boot.1/default/7e0ca7b10ef8102e3e459b6a8de1b8d09f8c700e82e97d2af2c1d940d967c9ab/0, testarg=foo, thirdkarg=bar]
Verifying karg testarg=foo
Verifying karg thirdkarg=baz
Error:   × Assertion failed.
     ╭─[/var/ARTIFACTS/work-test-20-local-upgradem4wulh6_/tmt/plans/integration/test-20-local-upgrade/discover/default-0/tests/tmt/tests/booted/test-image-pushpull-upgrade.nu:165:16]
 164 │         print $"Verifying karg ($x)"
 165 │         assert ($x in $cmdline)
     ·                ────────┬───────
     ·                        ╰── It is not true.
 166 │     }
     ╰────

[jeckersb]

/gemini review

[gemini-code-assist]

Code Review

This pull request is a nice refactoring to use a dedicated Cmdline type for kernel arguments instead of Vec<String>, which improves type safety and code clarity. The implementation is solid, introducing a new remove_exact method and updating various parts of the codebase to use the new type. I've found one bug in how root arguments are filtered and have a suggestion to reduce code duplication in the tests by using a helper function. Overall, great work on improving the codebase.

[jeckersb]

Also we should modify the upgrade integration test to include quoted kargs as in ostreedev/ostree#3544 which originally prompted all of this

[jeckersb]

Ok good news is that cherry-picking the patch that modifies the upgrade test onto main reproduces the original problem where ostree aborts:

16:32:23                         content:  INFO Staging image for deployment: ostree-unverified-image:containers-storage:localhost/bootc-derived (digest: sha256:84c3e696405c96ad1a4ed6f122f5588a14dbe4bdf78e70999a1fb75e0599ce70) message_id="8f7a2b1c3d4e5f6a7b8c9d0e1f2a3b4c" bootc.image.reference="localhost/bootc-derived" bootc.image.transport="containers-storage" bootc.manifest_digest="sha256:84c3e696405c96ad1a4ed6f122f5588a14dbe4bdf78e70999a1fb75e0599ce70"
16:32:23                         content: DEBUG kargs: added=["thirdkarg=baz"] removed=["othertestkarg", "thirdkarg=bar"]
16:32:23                         content: Deploying...**
16:32:23                         content: OSTree:ERROR:src/libostree/ostree-kernel-args.c:197:split_kernel_args: 'quoted' should be FALSE
16:32:23                         content: Bail out! OSTree:ERROR:src/libostree/ostree-kernel-args.c:197:split_kernel_args: 'quoted' should be FALSE
16:32:23                         content: Error: nu::shell::core_dumped
16:32:23                         content:
16:32:23                         content:   x External command core dumped
16:32:23                         content:      ,-[/var/tmp/tmt/run-019/tmt/plans/integration/test-20-local-upgrade/discover/default-0/tests/tmt/tests/booted/test-image-pushpull-upgrade.nu:160:20]
16:32:23                         content:  159 |     # We should already be fetching updates from container storage
16:32:23                         content:  160 |     RUST_LOG=debug bootc upgrade
16:32:23                         content:      :                    ^^|^^
16:32:23                         content:      :                      `-- core dumped with SIGABRT (6)
16:32:23                         content:  161 |     # Verify we staged an update
16:32:23                         content:      `----

[cgwalters]

One thing I'm questioning but otherwise looks sane

[cgwalters]

Looks good though one thing to bear in mind is that we can push string parsing all the way up into clap structs where applicable, and I think this is one of those cases

[jeckersb]

Yeah that would be ideal. It's a bit awkward the way things are right now though, because Parameter is always borrowed from elsewhere (typically from its parent Cmdline). I guess we could refactor it to work exactly like Cmdline so it uses a Cow internally and then we could build owned values from the clap options.

[jeckersb]

Yeah that would be ideal. It's a bit awkward the way things are right now though, because Parameter is always borrowed from elsewhere (typically from its parent Cmdline). I guess we could refactor it to work exactly like Cmdline so it uses a Cow internally and then we could build owned values from the clap options.

Actually after sleeping on it, it occurs to me that we can just use Option<Vec<CmdlineOwned>> for the karg flag. As a side-effect of that you would be able to specify multiple parameters in one option while still supporting multiple options. So for example: --karg "foo bar baz" --karg "wuz" would correctly parse that as three and one arg respectively, and add all four.

[jeckersb]

Hmm

thread 'main' (60193) panicked at crates/tests-integration/src/system_reinstall.rs:104:17:
File not found: /ostree/deploy/default/deploy/fee15478094fcc50471aad464eb2c51f8fb180c05b659ec22f11b1b270c831a1.0/usr/lib/bootc/fedora-bootc-destructive-cleanup
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
/home/runner/work/_temp/1fcac6ae-bc62-420f-bb5d-9c76858f2d28.sh: line 32: 60192 Aborted                 sudo bootc-integration-tests system-reinstall localhost/bootc-integration --test-threads=1
Error: Process completed with exit code 134.

That looks unrelated to this but worth noting.

[jeckersb]

Hmm

thread 'main' (60193) panicked at crates/tests-integration/src/system_reinstall.rs:104:17:
File not found: /ostree/deploy/default/deploy/fee15478094fcc50471aad464eb2c51f8fb180c05b659ec22f11b1b270c831a1.0/usr/lib/bootc/fedora-bootc-destructive-cleanup
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
/home/runner/work/_temp/1fcac6ae-bc62-420f-bb5d-9c76858f2d28.sh: line 32: 60192 Aborted                 sudo bootc-integration-tests system-reinstall localhost/bootc-integration --test-threads=1
Error: Process completed with exit code 134.

That looks unrelated to this but worth noting.

Ah I see #1748 now 👍