Add a hidden option to ostree admin kargs edit-in-place to update all existing deployments in place

[HuijingHei]

Describe the enhancement

Add a hidden option to ostree admin kargs edit-in-place to update all existing deployments in place.

Example:

$ sudo ostree admin kargs edit-in-place --append-if-missing=rw
This would be useful for: https://fedoraproject.org/wiki/Changes/Silverblue_Kinoite_readonly_sysroot

[travier]

Not a full review but looks good.

[cgwalters]

Thanks so much for working on this!

[HuijingHei]

• The value that has = like TESTARG=TESTVALUE will be appended again if call twice, for example the result will be like:

# options root=LABEL=MOO quiet ostree=/ostree/boot.1/testos/7bd091ad6955c0e7ef41bab65adb016275664b55efc0d10a0c8cb263da3c1e4e/0 ARGWITHOUTKEY TESTARG=TESTVALUE TESTARG=TESTVALUE

• But value like ARGWITHOUTKEY will not be appended twice as expected.

[HuijingHei]

Parse the key from key=value and call g_hash_table_contains (kargs->table, key)

[cgwalters]

Right...I think the logic we want here is more like:

  g_autofree char *key = g_strdup (arg);
  const char *val = split_keyeq (key);

  // Don't insert a duplicate key.
  if (g_hash_table_contains (kargs->table, key))
    return; 

  ostree_kernel_args_append (kargs, arg);

or so?

Though, if the argument table already has e.g. TESTARG=VAL1 and we say append_if_missing (TESTARG=VAL2) ...do we expect it to be ignored, or to add a new value, or to replace the existing one? I guess in most cases we want it to be ignored, which is what will happen here.

[HuijingHei]

/retest

[cgwalters]

Right...I think the logic we want here is more like:

  g_autofree char *key = g_strdup (arg);
  const char *val = split_keyeq (key);

  // Don't insert a duplicate key.
  if (g_hash_table_contains (kargs->table, key))
    return; 

  ostree_kernel_args_append (kargs, arg);

or so?

Though, if the argument table already has e.g. TESTARG=VAL1 and we say append_if_missing (TESTARG=VAL2) ...do we expect it to be ignored, or to add a new value, or to replace the existing one? I guess in most cases we want it to be ignored, which is what will happen here.

[HuijingHei]

You are right, this will not add duplicate key.

Update according to your suggestion, and make a little change for const char *val = split_keyeq (key); to split_keyeq (key); to avoid error

src/libostree/ostree-kernel-args.c:824:15: warning: unused variable 'val' [-Wunused-variable]
  824 |   const char *val = split_keyeq (key);

[cgwalters]

 --- expected-documented.txt	2022-06-23 03:30:28.690917580 +0000
+++ found-documented.txt	2022-06-23 03:30:28.694917654 +0000
@@ -116,7 +116,6 @@
 ostree_kernel_args_append
 ostree_kernel_args_append_argv
 ostree_kernel_args_append_argv_filtered
-ostree_kernel_args_append_if_missing
 ostree_kernel_args_append_proc_cmdline
 ostree_kernel_args_cleanup
 ostree_kernel_args_delete
OK closing connection
ERROR: tests/test-symbols.sh - too few tests run (expected 3, got 1)
ERROR: tests/test-symbols.sh - exited with status 1

Need to also add the new symbol to apidoc/ostree-sections.txt.

[HuijingHei]

Need to also add the new symbol to apidoc/ostree-sections.txt.

Update is done, thanks!

[cgwalters]

Thanks!

[cgwalters]

/override continuous-integration/jenkins/pr-merge

[openshift-ci]

@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/jenkins/pr-merge

In response to this:

/override continuous-integration/jenkins/pr-merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[travier]

🎉

[travier]

Thanks for the work here @HuijingHei !

[HuijingHei]

I am much appreciated with the help of @cgwalters and @travier, thanks a lot!