rpm-ostree kargs command #1013

peterbaouoft · 2017-09-26T13:57:29Z

This is initial groundwork for #594.

This commit sets up most of the required
front end logic( arg parsing, transaction handling), and will
be used in the following commits.

There is nothing really fancy in this commit, as most of the code
shares the similar style between other dbus related commands.

cgwalters

Looks like a good start! Thanks a ton for working on this! Some early feedback here.

cgwalters · 2017-09-26T14:00:00Z

src/app/rpmostree-builtin-kargs.c

+
+  g_variant_dict_init (&dict, NULL);
+  g_variant_dict_insert (&dict, "merge", "b", opt_merge);
+  g_variant_dict_insert (&dict, "editor", "b", opt_editor);


Editor should be purely client side, right? I can't think of a reason to send it to the daemon offhand.

cgwalters · 2017-09-26T14:03:08Z

src/daemon/org.projectatomic.rpmostree1.xml

+        "editor" (type 'b')
+        "merge" (type 'b')
+    -->
+    <method name="KernelArgs">


This feels close to the SetInitramfsState method - I don't see a reason for it to be experimental either. So let's move this right underneath that method in the XML, and also implement it close by in rpmostreed-os.c?

We might actually consider a general SetBootConfiguration method, but we don't need to do that right now. Maybe though internally route os_handle_set_initramfs_state() and os_handle_set_boot_args() into an internal-only os_handle_boot_configuration() method or so at least? Like how we route the pkg install/deploy etc. into a generic os_merge_or_start_deployment_txn().

(And really perhaps down the line we need to be able to both do initramfs/kargs/deploy all in one, but no need to worry about that now)

cgwalters · 2017-09-26T14:15:47Z

src/daemon/org.projectatomic.rpmostree1.xml

+    <method name="KernelArgs">
+      <arg type="as" name="kernel_args_added" direction="in"/>
+      <arg type="as" name="kernel_args_replaced" direction="in"/>
+      <arg type="as" name="kernel_args_removed" direction="in"/>


The semantics here get interesting because kernel arguments are a multimap. For example dracut parses rd.lvm.lv=foo/bar rd.lvm.lv=baz/moo etc. So if I specify rd.lvm.lv=hello/world in kernel_args_replaced, does that eliminate all of the other rd.lvm.lv args?

There's also a similar discussion for ostree remotes in ostreedev/ostree#1166 (comment)

So if I specify rd.lvm.lv=hello/world in kernel_args_replaced, does that eliminate all of the other rd.lvm.lv args

I believe so, because my original plan for backend logic was to reuse the parsing functions for OstreeKernelArgs( _ostree_kernel_args_append_kargv and _ostree_kernel_args_replace_kargv ) to parse all of the kernel arguments. And the old kernel arg parsing logic uses a hashtable to replace the duplicate keys, so other rd.lvm.lv args existed earlier will be removed.

This is the original code https://github.com/ostreedev/ostree/blob/master/src/ostree/ot-admin-instutil-builtin-set-kargs.c#L97 and https://github.com/ostreedev/ostree/blob/master/src/libostree/ostree-kernel-args.c#L100

Do we want that?

peterbaouoft · 2017-09-30T00:17:15Z

src/libpriv/rpmostree-kargs-process.c

@@ -0,0 +1,331 @@
+/*


Note: This file is copied straight from https://github.com/ostreedev/ostree/blob/master/src/libostree/ostree-kernel-args.c, just for testing the functionality purposes. Need to decide whether or not we should make the ostree-kernel-args public.

Also Note: In this file, functions I newly added will be commented with Note: this function is newly added to the API on top of the function. The other functions without those are from the old existing api.

peterbaouoft · 2017-09-30T00:17:37Z

src/libpriv/rpmostree-kargs-process.h

@@ -0,0 +1,60 @@
+/*


Note: Also same, copied from ostree for testing purposes

peterbaouoft · 2017-10-03T16:42:18Z

Hey, there are a few design decisions that I am currently uncertain about, and like to ask them here. So.. thanks in advance :p.

1: Do we want to make the functions/API in ostree kargs public, or it is fine by copying these to rpm-ostree side directly. Reason why I asked is I am also adding some functions to the existing API to achieve 'delete' and 'replace' related functionality, and unsure where to put it.

2: For the replace option. There is a special case that there could be multiple values associated with one key. In that case, we only want the user to replace one of them, and we may want the user to type replace the key by providing key=value=new_value. But it feels a bit messy that way because of the two equal signs.. so wondering is it fine that I go with (rpm-ostree ex kargs --replace key=value=new_value) implementation for now?

3: When we do append/replace option, there could be a case that users append or replace a kernel argument with exact key/value pair. e.g: rpm-ostree ex kargs --append hi=hi --append hi=hi. Do we need/want to worry about that case?

@cgwalters thoughts?

cgwalters · 2017-10-04T13:44:28Z

For question 1) I'm fine copying them into rpm-ostree for now. We can always dedup later. But it's up to you...we can add APIs to libostree too. Maybe on the OstreeDeployment class?

For question 2) --replace key=value=new_value seems good to me!

For question 3) Let's not make that an error...though it is yet another corner case. Hmm, I wonder if anything in the kernel actually uses that. Probably not...but I'm not 100% sure.

peterbaouoft · 2017-10-05T19:38:09Z

Currently the main functionality should be done (I think) and here is the introduction to some of the functionalities. I am sure there are still many things that are not polished, and the comments/commit messages need to be reworked too, but wanted to see what your opinions are first :). I am still an inexperienced coder, so there might still be a lot things that I did wrong, so feel free to point out the flaws/errors/bad code out =). And most importantly, thanks for your time :).

Here is the introduction to the functionalities :D: Note, by default, the kernel arguments are first collected/merged from the first/pending deployment' kernel args in the system.
==============================Kargs=====================================

[rbao@unused-10-15-17-147 f26AH]$ vagrant ssh
Last login: Thu Oct  5 18:27:15 2017 from 192.168.121.1
[vagrant@localhost ~]$ sudo rpm-ostree status
State: idle
Deployments:
* vmcheck
                 Timestamp: 2017-10-05 18:38:22
                    Commit: f034e215cce4fa667cdfb53692232da947577e81d61ba4ac892cbd133aaaf980
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs
The kernel arguments are:
no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii

[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf  
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii

===============================Kargs append===============================

[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf  
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii
[vagrant@localhost ~]$ sudo rpm-ostree status
State: idle
Deployments:
* vmcheck
                 Timestamp: 2017-10-05 18:38:22
                    Commit: f034e215cce4fa667cdfb53692232da947577e81d61ba4ac892cbd133aaaf980
// append will create a new pending deployment
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --append test=test
Copying /etc changes: 24 modified, 0 removed, 141 added
Transaction complete; bootconfig swap: yes deployment count change: 1
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf  
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.0/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii test=test
[vagrant@localhost ~]$ sudo rpm-ostree status
State: idle
Deployments:
  vmcheck
                 Timestamp: 2017-10-05 18:38:22
                    Commit: f034e215cce4fa667cdfb53692232da947577e81d61ba4ac892cbd133aaaf980

* vmcheck
                 Timestamp: 2017-10-05 18:38:22
                    Commit: f034e215cce4fa667cdfb53692232da947577e81d61ba4ac892cbd133aaaf980

// test multiple appends
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --append hello --append test=again --append=this=test
Copying /etc changes: 24 modified, 0 removed, 141 added
Transaction complete; bootconfig swap: yes deployment count change: 0
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii test=test test=again hello this=test

// Finally we can do a rollback
[vagrant@localhost ~]$ sudo rpm-ostree rollback 
Moving 'f034e215cce4fa667cdfb53692232da947577e81d61ba4ac892cbd133aaaf980.0' to be first deployment
Transaction complete; bootconfig swap: yes deployment count change: 0
Run "systemctl reboot" to start a reboot
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.0/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii

===========================Kargs Delete===================================

[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.0/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii
// delete one to one key/value pair
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --delete hi --delete no_timer_check
Copying /etc changes: 24 modified, 0 removed, 142 added
Transaction complete; bootconfig swap: yes deployment count change: 0
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0

//delete key with multiple values: 
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --delete console
error: Unable to delete console with multiple values associated with it, see rpm-ostree ex kargs -h for usage
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --delete console=tty1
Copying /etc changes: 24 modified, 0 removed, 142 added
Transaction complete; bootconfig swap: yes deployment count change: 0
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.0/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0

==============================Kargs Replace===================================

[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options no_timer_check console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hiii
// single pair replacement
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --replace no_timer_check=hello --replace hi=hmm
Copying /etc changes: 24 modified, 0 removed, 142 added
Transaction complete; bootconfig swap: yes deployment count change: 0
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options no_timer_check=hello console=tty1 console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.1/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hmm

// try replace a key with multiple values
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --replace console    
error: Unable to replace console with multiple values associated with it, see rpm-ostree ex kargs -h for usage
[vagrant@localhost ~]$ sudo rpm-ostree ex kargs --replace console=tty1=test
Copying /etc changes: 24 modified, 0 removed, 142 added
Transaction complete; bootconfig swap: yes deployment count change: 0
[vagrant@localhost ~]$ grep ^options /boot/loader/entries/ostree-fedora-atomic-0.conf
options no_timer_check=hello console=test console=ttyS0,115200n8 net.ifnames=0 biosdevname=0 rd.lvm.lv=atomicos/root root=/dev/mapper/atomicos-root ostree=/ostree/boot.0/fedora-atomic/aac51b16bc712956159c198c65180ef483f9dc8e0dd74f672f91bb365da5822b/0 hi=hmm

rh-atomic-bot · 2017-10-06T18:21:58Z

☔ The latest upstream changes (presumably dd7c748) made this pull request unmergeable. Please resolve the merge conflicts.

peterbaouoft · 2017-10-07T00:00:57Z

Rebased ⬆️ and fix most of the style issues that I spotted. Lifting WIP. :)

The editor command still is left undone because I am hesitant whether I should do a copy paste too :(. It seems like the logic will be exact same as: https://github.com/ostreedev/ostree/blob/master/src/ostree/ot-editor.c#L59. But... I tried, and.. this function is builtin if I am doing it correctly..

In addition to the code duplicate, I am also unsure about which RpmOstreeBuiltinFlags I should actually give to the rpm-ostree ex kargs. As we also want to enforce the editor option to be under root privilege too.

Also, there are many... fixup commits. I decided not to squash them yet, in case you want to see the progress. If it is way too messy, let me know, and the patch should be ok for review :)

cgwalters · 2017-10-09T17:12:24Z

src/app/rpmostree-builtin-kargs.c

+  /* We do not want editor option to be handled via daemon */
+  if (opt_editor)
+    {
+      ;


Let's comment out the option until it's implemented?

cgwalters · 2017-10-09T17:12:53Z

src/app/rpmostree-builtin-kargs.c

+      char *empty_strv[] = {NULL};
+
+      /* dbus does not allow empty string arrays, assign it
+       * to be empty string array here to prevent erroring out */


"does not allow NULL to mean the empty string array" would be a bit more precise.

cgwalters · 2017-10-09T17:13:39Z

src/daemon/rpmostree-sysroot-upgrader.c

+ *
+ */
+gboolean
+rpmostree_sysroot_upgrader_deploy_set_kargs (RpmOstreeSysrootUpgrader *self,


I'm OK with this, but I think down the line we should add the kernel args to rpmostree_sysroot_upgrader_deploy() and have the other callers pass NULL.

hey thanks for the review :). For this part, I believe other callers are still able to set NULL for the kernel args, as the kargs_strv is implemented as one of the attributes from the upgrader, similar to other attributes as well.
https://github.com/projectatomic/rpm-ostree/pull/1013/files#diff-af8b24e3bf31175f007dd70d4b1a46fcR83

My personal opinion, but I could be wrong, is .. it feels a bit unnecessary to change other code when you can just set NULL to the kargs_strv attribute prior to calling rpmostree_sysroot_upgrader_deploy? ( I kinda tried to avoid changing too much existing code base too =) ). But I am also fine if you want to change the function.

cgwalters · 2017-10-09T17:15:54Z

src/daemon/rpmostreed-transaction-types.c

+  /* Set this boolean to have a default value */
+  gboolean import_from_cmd = FALSE;
+
+  if (self->flags & RPMOSTREE_TRANSACTION_KERNEL_ARG_FLAG_IMPORT_CMD)


I read "import cmd" as an abbreviation for "import command" - let's say "_IMPORT_CMDLINE" to be clearer that it's the "command line"? Or maybe to be even more clear, say "FLAG_IMPORT_PROC_CMDLINE" ? And name the boolean import_from_proc_cmdline.

cgwalters · 2017-10-09T17:22:00Z

src/daemon/rpmostreed-transaction-types.c

+
+  /* When none of the arguments are present, we print out the merged/current kernel arguments and leave */
+  if (!(self->kernel_args_added) && !(self->kernel_args_deleted)
+      && !(self->kernel_args_replaced))


It feels a bit weird to do this on the server side; it turns a conceptually read-only operation into one that allocates a transaction and e.g. blocks other transactions.

I was thinking we add the kernel args to the DBus properties but then I realized that they might contain secret data. So perhaps we add a GetDeploymentBootconfig DBus method?

Going this route seems required for implementing the editor path anyways, right?

By an example, look at rpmostree-builtin-initramfs.c which just reads the DBus properties on the client to find out the enabled state, and the "print" lives there.

cgwalters · 2017-10-09T17:28:15Z

tests/vmcheck/test-kernel-args.sh

+vm_cmd grep ^options /boot/loader/entries/ostree-$osname-0.conf > tmp_conf.txt
+assert_not_file_has_content 'REPLACE_MULTI_TEST=NUMBERTWO'
+assert_not_file_has_content 'REPLACE_MULTI_TEST=NEWTEST'
+assert_not_file_has_content 'APPENDARG=2NDAPPEND'


Looks like these are missing a tmp_conf.txt argument?

yup, thanks for catching it :). The weird part is it did not error out too.... Will be more careful next time.

cgwalters · 2017-10-09T17:29:37Z

While it's sometimes useful to see the development stream, at this point I think we should squash the patches together and do further fixups on top.

This is initial groundwork for coreos#594. This commit sets up most of the required front end logic( arg parsing, transaction handling), and will be used in the following commits. There is nothing really fancy in this commit, as most of the code shares the similar style between other dbus related commands.

API functions from ostree-kernel-args.c are copied to libpriv. The append functionality reuses _ostree_kernel_args_append_argv() for collecting added kernel arguments. Also added handlation in rpm-ostree upgrader to allow deployments happen with kernel arguments. Now, the user is able to add kernel arguments via 'rpm-ostree ex kargs --append key=value' or 'rpm-ostree ex kargs --append key' if they want to have an empty value with key. The user is also able to display the current kernel arguments via 'rpm-ostree ex kargs' In addition, this functionality will create a pending deployment, and will update the conf file in /boot/loader/entries/ostree-$osname-0.conf upon success.

Some new functions are added to handle delete operations for kargs. We are now able to do the following delete operations kargs: 1: delete by key, if there is only one value associated with the key we directly remove it 2: delete by key=value, it will find that specific key/value pair and remove it from the kernel arguments Similar to append functionality introduced, delete will also create a new pending deployment that is rollbackable

Tests are introduced in this commit to test the basic functionality for rpm-ostree ex kargs --append, and rpm-ostree ex kargs --delete. Those tests are added for future regression.

Refactor the similar code portion from delete option into a new function, and reuse it in the replace logic. Add the replace functionality to allow kernel arguments replacement. Now support two different types of replacement: 1: replace by key=value when there is only one key existing e.g we have 'hi=hello' as our one of the arguments (hi only has one value associated with it) , and 'rpm-ostree ex kargs --replace hi=new' will replace the value and change hi=hello into hi=new. 2: replace by key=value=new_value for all other replacements ( this will work for swapping single value pair too !) Some tests for rpm-ostree ex kargs --replace are added for future regression.

peterbaouoft · 2017-10-11T19:56:28Z

Added a fixup to address the comments above, ⬆️, the commits are also auto squashed. Do note that there are also some extra code introduced to fix/improve the existing functionality.

1: in the src/libpriv/rpmostree-kargs-process.c, an extra corner case handlation was added to fix the replace logic

2:

I was thinking we add the kernel args to the DBus properties but then I realized that they might contain secret data. So perhaps we add a GetDeploymentBootconfig DBus method?

This was implemented in the fix up. (not sure if it is a good place to put it yet), but decided to put the entire implementation into the fix up as well because some of the duplicate code used in the earlier commits can be removed. Now, we will first load the existing kernel arguments outside the kernel arguments transaction and pass it into the transaction, instead of loading it directly inside the transaction.

3: After the GetDeploymentBootconfig is implemented, a few extra options( reboot, deployid) were added in src/app/rpmostree-builtin-kargs.c. The import-from-cmd option is also no longer handled inside the transaction execute function.

4: There might be many things I did wrong that I am not aware of :(. So feel free to give comments so that I can fix them :). Thank you for your time :D!

cgwalters

This is looking pretty good overall! Just a few more comments:

cgwalters · 2017-10-23T13:31:44Z

src/app/rpmostree-builtin-kargs.c

+
+static GOptionEntry option_entries[] = {
+  { "os", 0, 0, G_OPTION_ARG_STRING, &opt_osname, "Operation on provided OSNAME", "OSNAME" },
+  { "deployid", 0, 0, G_OPTION_ARG_STRING, &opt_deployid, "Modify the kernel args from a specific deployment based on id. Id is in the form of 'osname-checksum.deployserialnum' ", "DEPLOYID"},


Interesting. This is a new "input type" - I think it's a good idea to have something like this, but it conflicts a bit with the package layering path, where we actually hide the final checksum. Right now the closest analogue to this we have is ostree admin undeploy N where N is an integer "deployment index". (Not super user friendly)

I think my tentative vote here is to instead have --from-rollback - that should cover the major case of "undo my kargs to whatever was booted last" right?

I see this is used in the DBus API too. Hmm. I'm not sure. I think my tentative vote here is to use an index in the API too but I could definitely be convinced to try to standardize this "deployid" too.

I think my tentative vote here is to use an index in the API too

Sure, index here makes more sense. I used the deploy id implementation just simply because this was also used in the old DBus methods :p. Will do a change.

cgwalters · 2017-10-23T13:35:47Z

src/app/rpmostree-builtin-kargs.c

+                                                error))
+    return EXIT_FAILURE;
+
+  g_print("The kernel arguments are now modified \n");


How about:

g_print ("Kernel arguments updated.\nRun \"systemctl reboot\" to start a reboot\n");

which is a bit more consistent with the other commands? I think it's important to be clear we're not modifying the current deployment by default (right?)

cgwalters · 2017-10-23T14:38:49Z

src/daemon/rpmostreed-os.c

+    }
+  OstreeBootconfigParser *bootconfig = ostree_deployment_get_bootconfig (target_deployment);
+
+  /* Note because boot config is a private structure.. currently I have no good way


This we should fix in libostree I think. But it doesn't need to be done to land this change.

cgwalters · 2017-10-23T14:41:45Z

src/daemon/org.projectatomic.rpmostree1.xml

+        "reboot" (type 'b')
+    -->
+    <method name="KernelArgs">
+      <arg type="s" name="existing_kernel_arg_string"/>


Is this value actually used by the daemon? It looks to me like we pass it all the way down but don't actually use it, rather it's just parsed on the client side. Is that correct? If so, let's just drop this from the DBus API?

Hey, thanks for the review :). We actually do use it, it is this line

__attribute__((cleanup(_ostree_kernel_args_cleanup))) OstreeKernelArgs *kargs = _ostree_kernel_args_from_string (self->existing_kernel_args);

We use this attribute so that we do not need to do the extra loading of the wanted kargs again in the transaction execute, as we have already done the loading in rpmostree_os_call_get_deployment_boot_config_sync function.

Ah, right OK.

peterbaouoft · 2017-10-23T15:33:14Z

src/app/rpmostree-builtin-kargs.c

+
+  const char* deploy_str = opt_deployid ?: "";
+  g_autoptr(GVariant) boot_config = NULL;
+  if (!rpmostree_os_call_get_deployment_boot_config_sync (os_proxy,


Just noticed via a bit more testing, the policy kit verification will be called twice since there are two different os methods being called. Will do a bit more further investigation on how to avoid that from happening.

peterbaouoft · 2017-10-23T20:56:40Z

Added a fixup to address the comments above, ⬆️, also added a fix for solving the duplicated policy kit authentication calls.

But the implementation for the id parsing, and the solution for duplicate policy kit seems a bit ugly.. But, can't seem to think for a better solution atm. So, suggestions are welcome, and thanks for your time :)

Also, it might be unnecessary, but this is a reminder that there were also a few extra functions added in
the old kernel arg api file( these were hidden in the earlier commits, not in this fixup). The added functions all begin with a comment Note: this function is newly added to the API. And the name of the added functions will be the following:

_ostree_ptr_array_find()
_ostree_kernel_arg_query_status()
_ostree_kernel_args_new_replace()
_ostree_kernel_args_delete()
_ostree_kernel_args_delete_key_entry()

Much nicer looking. Prep for more cleanup from coreos/rpm-ostree#1013

cgwalters · 2017-10-31T18:00:49Z

bot, retest this please

cgwalters · 2017-10-31T19:14:41Z

src/libpriv/rpmostree-kargs-process.c

+  if (!values)
+    {
+      g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                           "The key is not found, type rpm-ostree ex kargs to see what args are there");


One thing to remember is that the DBus API may also be used by e.g. Cockpit or some other tool. So while it's tempting to mention the command line in errors, it will be confusing if we're being driven by other tools.

So how about: Failed to find kernel argument '%s', arg ?

cgwalters · 2017-10-31T19:16:35Z

src/libpriv/rpmostree-kargs-process.c

+                                        TRUE, &value_index, error))
+    return FALSE;
+
+  g_autofree char *duped = g_strdup (arg);


We have a bit of a naming convention like "arg_owned" (honestly this is me being influenced by Rust).

cgwalters · 2017-10-31T19:19:49Z

src/libpriv/rpmostree-kargs-process.c

+   * then put new one back in */
+  char *old_element = (char *)g_ptr_array_index (values, value_index);
+  g_free (old_element);
+  old_element = NULL;


This is totally fine, though I try to use "move semantics" style calls like g_free (g_steal_pointer (&old_element));. But up to you. Or you could just g_autofree char *old_element ....

cgwalters · 2017-10-31T19:21:21Z

src/libpriv/rpmostree-kargs-process.c

+                            const char       *arg,
+                            GError           **error)
+{
+  /* Make a duplicate of the passed in arg for manipulation */


This comment looks out of place?

yup, nice catch!

cgwalters · 2017-10-31T19:44:01Z

src/libpriv/rpmostree-kargs-process.c

+  if (!g_hash_table_remove (kargs->table, key))
+    {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                   "Failed to remove %s from hashtable",


Hmm...how about:
``"Failed to find kernel argument %s'", key`?

cgwalters · 2017-10-31T19:44:29Z

src/libpriv/rpmostree-kargs-process.c

+  if (!_ostree_ptr_array_find (kargs->order, key,
+                              &key_index))
+    {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,


This should be an assertion failure, not a runtime error right? If we've done things correctly then if the arg exists in the table it'll be in the arrray, right?

yup, thanks for pointing it out.

cgwalters · 2017-10-31T19:44:33Z

src/libpriv/rpmostree-kargs-process.c

+    }
+  if (!g_ptr_array_remove_index (kargs->order, key_index))
+    {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,


cgwalters · 2017-10-31T19:57:14Z

src/libpriv/rpmostree-kargs-process.c

+               values->len !=1)
+        {
+          g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
+                       "Unable to %s %s with multiple values associated with it, see rpm-ostree ex kargs -h for usage",


"Unable to %s argument '%s' with multiple values", is_replaced ? "replace" : "delete", duped or so?

cgwalters · 2017-10-31T20:06:16Z

src/libpriv/rpmostree-kargs-process.c

+        }
+      /* Handle both no value case, and the case when inputting
+         key=value for a replacement */
+      else if ((!*val || (is_replaced && !*replaced_val)) &&


Maybe let's do e.g.
const gboolean key_only = !*val above?

cgwalters · 2017-10-31T20:06:56Z

src/libpriv/rpmostree-kargs-process.c

+       * there is only one single key, and the second val
+       * will now represent the new value (no second split
+       * will happen this case) */
+      else if (val && is_replaced && values->len == 1)


const gboolean single_value = values->len == 1 above too?

Hm...and does this need to be if (*val && ...? (With the extra *?) I

Hm...and does this need to be if (*val && ...? (With the extra *?)

yup, thanks for catching it. Did not notice it when I checked it.

cgwalters · 2017-10-31T20:08:33Z

Some new comments; I think we're closing in here. I'd feel better if we had some unit-style tests for the kernel argument parsing. Should be pretty easy to do right?

peterbaouoft · 2017-10-31T20:19:43Z

Yup, many thanks for the review :).

I'd feel better if we had some unit-style tests for the kernel argument parsing. Should be pretty easy to do right

Sure, I am going to start writing those, hopefully won't take so long. Haven't written a long enough unit-style tests yet :p.

One more thing, the editor functionality is also almost done, so it will be a bit weird to add fixup to the old commits along with it. (considering there are many fixups already). So do you mind I autosquash the fixup commits earlier( including the newly added editor commit). Then every fixup commits later will be built on top? @cgwalters

cgwalters · 2017-10-31T20:21:49Z

Yeah, squashing now is totally fine!

cgwalters · 2017-10-31T20:22:04Z

Or if you want we can do the editor as a follow-up?

peterbaouoft · 2017-10-31T20:24:54Z

Thanks for the fast response! :).

Or if you want we can do the editor as a follow-up?

Sure, we can do it as a follow-up. I will first start writing the fixup commit and the unit test in that case :p.

Added unit tests for rpm-ostree ex kargs --delete, --append and --replace. Also exposed two getter functions for kargs table and array so people can retrieve information from kargs. Also includes a minor fix for a bug caught by the unit test.

peterbaouoft · 2017-11-02T19:56:36Z

bot, retest this please

peterbaouoft · 2017-11-03T14:16:54Z

Pushed a fixup ⬆️, addressed the comments mentioned above. Also pushed a commit for unit tests. There a few points that I think are worth noting:

1: rpmostreed_deployment_get_for_index did not have a unit test with it. The ostree sysroot and deployment is hard to set up for the testing. Thus choosing not to do so.

2: Not sure if I need to do unit testing for the dbus/command line related functions.

3: I also need to expose the kargs' table through _ostree_kernel_arg_get_kargs_table and _ostree_kernel_arg_get_key_array. So that might not be too neat, so suggestion for alternative implementation are always welcome :).

cgwalters · 2017-11-06T18:49:33Z

OK, this all looks pretty good to me; there's some relatively minor stuff but not worth more round trips here. Let's get it in - since this is under ex we can always do fixes as a followup. Thanks a lot for working on this!

@rh-atomic-bot r+ aaf2ed1

rh-atomic-bot · 2017-11-06T18:49:38Z

⌛ Testing commit aaf2ed1 with merge 5ca9c73...

API functions from ostree-kernel-args.c are copied to libpriv. The append functionality reuses _ostree_kernel_args_append_argv() for collecting added kernel arguments. Also added handlation in rpm-ostree upgrader to allow deployments happen with kernel arguments. Now, the user is able to add kernel arguments via 'rpm-ostree ex kargs --append key=value' or 'rpm-ostree ex kargs --append key' if they want to have an empty value with key. The user is also able to display the current kernel arguments via 'rpm-ostree ex kargs' In addition, this functionality will create a pending deployment, and will update the conf file in /boot/loader/entries/ostree-$osname-0.conf upon success. Closes: #1013 Approved by: cgwalters

Some new functions are added to handle delete operations for kargs. We are now able to do the following delete operations kargs: 1: delete by key, if there is only one value associated with the key we directly remove it 2: delete by key=value, it will find that specific key/value pair and remove it from the kernel arguments Similar to append functionality introduced, delete will also create a new pending deployment that is rollbackable Closes: #1013 Approved by: cgwalters

Tests are introduced in this commit to test the basic functionality for rpm-ostree ex kargs --append, and rpm-ostree ex kargs --delete. Those tests are added for future regression. Closes: #1013 Approved by: cgwalters

Refactor the similar code portion from delete option into a new function, and reuse it in the replace logic. Add the replace functionality to allow kernel arguments replacement. Now support two different types of replacement: 1: replace by key=value when there is only one key existing e.g we have 'hi=hello' as our one of the arguments (hi only has one value associated with it) , and 'rpm-ostree ex kargs --replace hi=new' will replace the value and change hi=hello into hi=new. 2: replace by key=value=new_value for all other replacements ( this will work for swapping single value pair too !) Some tests for rpm-ostree ex kargs --replace are added for future regression. Closes: #1013 Approved by: cgwalters

Refined a bit for the previous written kargs test.( mainly for checking kargs after rollback ) Added tests for import-proc-cmdline and deploy-index option Closes: #1013 Approved by: cgwalters

Added unit tests for rpm-ostree ex kargs --delete, --append and --replace. Also exposed two getter functions for kargs table and array so people can retrieve information from kargs. Also includes a minor fix for a bug caught by the unit test. Closes: #1013 Approved by: cgwalters

rh-atomic-bot · 2017-11-06T19:13:41Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 5ca9c73 to master...

peterbaouoft · 2017-11-06T19:16:29Z

Many thanks for all of the meaningful review in the past month! :)

peterbaouoft changed the title ~~app/daemon: add groundwork for rpm-ostree kargs command~~ WIP:app/daemon: add groundwork for rpm-ostree kargs command Sep 26, 2017

peterbaouoft changed the title ~~WIP:app/daemon: add groundwork for rpm-ostree kargs command~~ WIP: rpm-ostree kargs command Sep 26, 2017

cgwalters added the WIP label Sep 26, 2017

cgwalters reviewed Sep 26, 2017

View reviewed changes

peterbaouoft commented Sep 30, 2017

View reviewed changes

src/libpriv/rpmostree-kargs-process.h

@@ -0,0 +1,60 @@

/*

Copy link

Contributor Author

peterbaouoft Sep 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Also same, copied from ostree for testing purposes

peterbaouoft force-pushed the add_dbus_api_change_kargs branch from 081c90d to 7a59c9a Compare October 6, 2017 23:40

peterbaouoft changed the title ~~WIP: rpm-ostree kargs command~~ rpm-ostree kargs command Oct 7, 2017

cgwalters reviewed Oct 9, 2017

View reviewed changes

peterbaouoft added 6 commits October 10, 2017 13:12

vmcheck: Add tests for rpm-ostree ex kargs delete and append

dec9621

Tests are introduced in this commit to test the basic functionality for rpm-ostree ex kargs --append, and rpm-ostree ex kargs --delete. Those tests are added for future regression.

fixup! libpriv/vmcheck: support replace option for karg command

cc96a5f

peterbaouoft force-pushed the add_dbus_api_change_kargs branch from 7a59c9a to cc96a5f Compare October 11, 2017 19:44

peterbaouoft added 2 commits October 13, 2017 13:19

fixup! libpriv/vmcheck: support replace option for karg command

e12dd41

fixup! libpriv/vmcheck: support replace option for karg command

86d9db9

cgwalters reviewed Oct 23, 2017

View reviewed changes

peterbaouoft commented Oct 23, 2017

View reviewed changes

fixup! libpriv/vmcheck: support replace option for karg command

7681df9

cgwalters added a commit to cgwalters/ostree that referenced this pull request Oct 24, 2017

tree-wide: Use autoptr for OstreeKernelArgs

29b37ae

Much nicer looking. Prep for more cleanup from coreos/rpm-ostree#1013

cgwalters removed the WIP label Oct 30, 2017

cgwalters reviewed Oct 31, 2017

View reviewed changes

peterbaouoft and others added 2 commits November 2, 2017 19:06

fixup! libpriv/vmcheck: support replace option for karg command

081923e

fixup! tests/check: Add unit tests for kargs options

71b1a12

fixup! tests/check: Add unit tests for kargs options

aaf2ed1

cgwalters added the reviewed/approved label Nov 6, 2017

rh-atomic-bot closed this in fa593af Nov 6, 2017

jlebon mentioned this pull request Apr 30, 2018

dbus api to change kernel arguments #594

Closed

peterbaouoft deleted the add_dbus_api_change_kargs branch June 20, 2018 14:26

jlebon mentioned this pull request Mar 21, 2019

Add public API for kargs ostreedev/ostree#1833

Closed

rpm-ostree kargs command #1013

rpm-ostree kargs command #1013

Conversation

peterbaouoft commented Sep 26, 2017

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Oct 3, 2017

cgwalters commented Oct 4, 2017

peterbaouoft commented Oct 5, 2017

rh-atomic-bot commented Oct 6, 2017

peterbaouoft commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Oct 9, 2017

peterbaouoft commented Oct 11, 2017 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbaouoft commented Oct 23, 2017 • edited Loading

cgwalters commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Oct 31, 2017

peterbaouoft commented Oct 31, 2017

cgwalters commented Oct 31, 2017

cgwalters commented Oct 31, 2017

peterbaouoft commented Oct 31, 2017

peterbaouoft commented Nov 2, 2017

peterbaouoft commented Nov 3, 2017

cgwalters commented Nov 6, 2017

rh-atomic-bot commented Nov 6, 2017

rh-atomic-bot commented Nov 6, 2017

peterbaouoft commented Nov 6, 2017

peterbaouoft Oct 11, 2017 •

edited

Loading

peterbaouoft commented Oct 11, 2017 •

edited

Loading

peterbaouoft commented Oct 23, 2017 •

edited

Loading