kernel_cmdline: Add parameter manipulation methods

[jeckersb]

Add add_or_modify and remove methods to Cmdline in both bytes
and utf8 modules, along with unit tests.

Closes: #1596
Signed-off-by: John Eckersberg jeckersb@redhat.com

[gemini-code-assist]

Code Review

This pull request adds useful methods (add_or_modify and remove) for manipulating kernel command line parameters in both bytes and utf8 modules, along with corresponding unit tests. The overall approach is sound.

My review includes suggestions to refactor the implementations of add_or_modify and remove in the bytes module. The proposed changes aim to improve code clarity by replacing closures with side effects with more explicit for loops. This also addresses a bug related to handling empty command lines and removes some redundant code. The rest of the changes look good.

[jeckersb]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces add_or_modify and remove methods for manipulating kernel command line parameters, along with corresponding unit tests. The changes are a good addition, but I've identified a critical issue in the add_or_modify implementation concerning the handling of duplicate keys, which could lead to incorrect command line generation. I've provided a corrected implementation that resolves this and another minor bug. Additionally, I've pointed out a small redundancy and suggested improvements to test coverage to make the new functionality more robust.

[jeckersb]

interestingly enough i forgot to add my local changes and it generated a wildly different review for the same code 😆

[jeckersb]

/gemini review

[gemini-code-assist]

Code Review

This pull request adds useful methods for manipulating kernel command line parameters. The implementation of add_or_modify has a bug related to handling duplicate keys and another minor issue with appending to an empty command line. I've provided a detailed comment with a suggested fix. The rest of the changes look good.

[jeckersb]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces add_or_modify and remove methods for manipulating kernel command line parameters in both bytes and utf8 modules, complete with comprehensive unit tests. The implementation is robust and handles various edge cases like duplicate parameters and empty command lines. The code quality is high, and the changes are a valuable addition. I have one suggestion to improve the readability of the add_or_modify function's control flow.

[jeckersb]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces useful methods for manipulating kernel command line parameters, add_or_modify and remove, for both bytes and utf8 variants. The implementation is generally solid and includes a good set of unit tests to cover various scenarios. I've identified a minor robustness issue when appending new parameters and have also suggested an API improvement to enhance encapsulation and test clarity. Overall, these are great additions to the library.

[cgwalters]

I wonder if for the use cases that require mutation we should just create an ordered hashmap from it?

That's basically how https://github.com/ostreedev/ostree/blob/main/src/libostree/ostree-kernel-args.c works.

[cgwalters]

I guess this relates to #1608 (comment) in that how duplicate keys should be handled when setting a parameter.

[jeckersb]

Yeah in the current state (as in how this patch is at this very moment...) if you construct a Cmdline with duplicate parameters, we allow that and don't try to do any verification at build time. But if you remove a duplicate, all duplicates are removed. And if you add_or_modify a duplicate, it removes all of the old instances and replaces it with just the single new one. I think that makes sense.

[jeckersb]

re: "ordered hashmap", we'd have to pull in something like IndexMap? I see ostree-ext already uses that...

[cgwalters]

Yep indexmap is very widely used, I think it'd make sense here. Though we do need to preserve multiple keys so we should map from &str -> Vec<&str>

[jeckersb]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces add_or_modify and remove methods for manipulating kernel command line parameters, along with corresponding unit tests. The changes are well-implemented and the new functionality is useful. The utf8 module correctly delegates to the bytes module, promoting code reuse. The tests are comprehensive and cover various edge cases. I have one suggestion to improve the performance and clarity of the add_or_modify method for the case where a new parameter is being added.

[gemini-code-assist]

The current implementation of add_or_modify is a single-pass algorithm, which is efficient for modifications. However, when adding a new parameter (i.e., the key does not already exist), the implementation iterates through all existing parameters and builds a new_params vector, which is then discarded. This results in unnecessary allocation and processing.

A more optimal approach for the 'add' case would be to first check if the key exists. If it doesn't, you can take a fast path to append the new parameter without building the intermediate new_params vector. This change would improve performance for additions and make the code's intent clearer by separating the 'add' and 'modify' logic paths.

Here is a suggested refactoring that implements this approach:

    pub fn add_or_modify(&mut self, param: Parameter) -> bool {
        // By checking for existence first, we can fast-path the common case of adding a new parameter.
        if !self.iter().any(|p| p.key == param.key) {
            // The parameter was not found, so we append it.
            let self_mut = self.0.to_mut();
            if !self_mut.is_empty() && !self_mut.last().unwrap().is_ascii_whitespace() {
                self_mut.push(b' ');
            }
            self_mut.extend_from_slice(param.parameter);
            return true;
        }

        // The key exists, so we need to rebuild.
        let mut new_params = Vec::new();
        let mut modified = false;
        let mut seen_key = false;

        for p in self.iter() {
            if p.key == param.key {
                if !seen_key {
                    // This is the first time we've seen this key.
                    // We will replace it with the new parameter.
                    if p != param {
                        modified = true;
                    }
                    new_params.push(param.parameter);
                } else {
                    // This is a subsequent parameter with the same key.
                    // We will remove it, which constitutes a modification.
                    modified = true;
                }
                seen_key = true;
            } else {
                new_params.push(p.parameter);
            }
        }

        if modified {
            self.0 = Cow::Owned(new_params.join(b" ".as_slice()));
            true
        } else {
            // The parameter already existed with the same content, and there were no duplicates.
            false
        }
    }

[jeckersb]

Meh, constructing the vec isn't that expensive, it's just copying the slice references. I purposefully tried to avoid iterating the whole thing twice, but kinda indifferent on this one.

[cgwalters]

Yeah we shouldn't care about performance here, only correctness and maintainability