kargs: run delete-if-present and append-if-missing failed when there is existing key
#4738
Conversation
|
Skipping CI for Draft Pull Request. |
HuijingHei
force-pushed
the
revert-kargs-delete
branch
4 times, most recently
from
317a497 to
f0d9d87
Compare
HuijingHei
force-pushed
the
revert-kargs-delete
branch
from
f0d9d87 to
b5aa8c3
Compare
|
Can you squash into a single commit too? |
HuijingHei
force-pushed
the
revert-kargs-delete
branch
2 times, most recently
from
68ed68a to
eb4013e
Compare
when there is existing `key` Revert part of coreos@1a1ef13 `ostree_kernel_args_delete_if_present()` calls `ostree_kernel_args_contains()` which only looks at the `key` part, which makes `delete-if-present` failed when there is existing `key`. `append-if-missing` will hit the same problem. ``` ]# rpm-ostree kargs --append=crashkernel=1G ]# rpm-ostree kargs ... crashkernel=1G ]# rpm-ostree kargs --unchanged-exit-77 --delete-if-present=crashkernel=auto error: No karg 'crashkernel=auto' found ]# rpm-ostree kargs --unchanged-exit-77 --append-if-missing=crashkernel=auto No changes. ``` And fix corner case when `append` and `append-if-missing` with the same value which will append twice. Fixes: coreos#4718
HuijingHei
force-pushed
the
revert-kargs-delete
branch
from
eb4013e to
cdd56d6
Compare
Sure, done. Thanks for the review! |
There was a problem hiding this comment.
My main current fear right here is that we had a set of behaviors before #4161 landed - and now have been shipping for a while with new behaviors. I'm a bit worried that we're potentially introducing a new 3rd variation of things - i.e. I'm a bit worried there might be cases that are relying on the current behavior that break with this.
However: the fact that this new code passes the existing units tests gives some confidence at least. (Offhand looking at things it seems to me we could pretty easily create new unit tests for kernel_arg_apply_patching too that run outside of a VM, would be way way cheaper to test)
OK well, it seems like this PR is basically a clean revert of that change, alongside a fix for idempotent append append-if-missing...
| @@ -2707,19 +2710,20 @@ kernel_arg_apply_patching (KernelArgTransaction *self, RpmOstreeSysrootUpgrader | |||
| for (char **iter = append_if_missing; iter && *iter; iter++) | |||
| { | |||
| const char *arg = *iter; | |||
| if (!ostree_kernel_args_contains (kargs, arg)) | |||
There was a problem hiding this comment.
I think this would be cleaner with an ostree_kernel_args_contains_value which doesn't strip off the value.
Revert part of 1a1ef13
ostree_kernel_args_delete_if_present()callsostree_kernel_args_contains()which only looks at thekeypart, which makes
delete-if-presentfailed when there isexisting
key.append-if-missingwill hit the same problem.And fix corner case when
appendandappend-if-missingwiththe same value which will append twice.
Fixes: #4718