From 06d39efcb5ad52216a2a47b5eff71af82c0dae49 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Fri, 27 Apr 2018 11:58:50 -0400
Subject: [PATCH] bin/deploy: Avoid loading merge deployment kargs unless
 necessary

The fact that `ostree admin deploy` always itself loaded the
merge kargs masked a bug in the core.  Let's change our tests
to not pass any kernel arguments to ensure we cover this.

The new logic in the CLI is a bit subtle, but if you read
carefully is a lot clearer I believe.  Basically we have one
of a few "starting points" in the first section, which can
then be further augmented.

Closes: #1558
Approved by: jlebon
---
 src/ostree/ot-admin-builtin-deploy.c          | 30 ++++++++++++-------
 tests/installed/destructive/staged-deploy.yml | 13 +++-----
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/src/ostree/ot-admin-builtin-deploy.c b/src/ostree/ot-admin-builtin-deploy.c
index f6c0c161..0a1755fd 100644
--- a/src/ostree/ot-admin-builtin-deploy.c
+++ b/src/ostree/ot-admin-builtin-deploy.c
@@ -65,8 +65,6 @@ static GOptionEntry options[] = {
 gboolean
 ot_admin_builtin_deploy (int argc, char **argv, OstreeCommandInvocation *invocation, GCancellable *cancellable, GError **error)
 {
-  g_autoptr(OstreeKernelArgs) kargs = NULL;
-
   g_autoptr(GOptionContext) context =
     g_option_context_new ("REFSPEC");
 
@@ -129,36 +127,48 @@ ot_admin_builtin_deploy (int argc, char **argv, OstreeCommandInvocation *invocat
   if (!ostree_sysroot_prepare_cleanup (sysroot, cancellable, error))
     return glnx_prefix_error (error, "Performing initial cleanup");
 
-  kargs = _ostree_kernel_args_new ();
-
-  /* If they want the current kernel's args, they very likely don't
-   * want the ones from the merge.
+  /* Initial set of kernel arguments; the default is to use the merge
+   * deployment, unless --karg-none or --karg-proc-cmdline are specified.
    */
-  if (opt_kernel_proc_cmdline)
+  g_autoptr(OstreeKernelArgs) kargs = NULL;
+  if (opt_kernel_arg_none)
     {
+      kargs = _ostree_kernel_args_new ();
+    }
+  else if (opt_kernel_proc_cmdline)
+    {
+      kargs = _ostree_kernel_args_new ();
       if (!_ostree_kernel_args_append_proc_cmdline (kargs, cancellable, error))
         return FALSE;
     }
-  else if (merge_deployment && !opt_kernel_arg_none)
+  else if (merge_deployment && (opt_kernel_argv || opt_kernel_argv_append))
     {
       OstreeBootconfigParser *bootconfig = ostree_deployment_get_bootconfig (merge_deployment);
       g_auto(GStrv) previous_args = g_strsplit (ostree_bootconfig_parser_get (bootconfig, "options"), " ", -1);
-
+      kargs = _ostree_kernel_args_new ();
       _ostree_kernel_args_append_argv (kargs, previous_args);
     }
 
+  /* Now replace/extend the above set.  Note that if no options are specified,
+   * we should end up passing NULL as override_kernel_argv for
+   * ostree_sysroot_deploy_tree() so we get the defaults.
+   */
   if (opt_kernel_argv)
     {
+      if (!kargs)
+        kargs = _ostree_kernel_args_new ();
       _ostree_kernel_args_replace_argv (kargs, opt_kernel_argv);
     }
 
   if (opt_kernel_argv_append)
     {
+      if (!kargs)
+        kargs = _ostree_kernel_args_new ();
       _ostree_kernel_args_append_argv (kargs, opt_kernel_argv_append);
     }
 
   g_autoptr(OstreeDeployment) new_deployment = NULL;
-  g_auto(GStrv) kargs_strv = _ostree_kernel_args_to_strv (kargs);
+  g_auto(GStrv) kargs_strv = kargs ? _ostree_kernel_args_to_strv (kargs) : NULL;
   if (opt_stage)
     {
       if (opt_retain_pending || opt_retain_rollback)
diff --git a/tests/installed/destructive/staged-deploy.yml b/tests/installed/destructive/staged-deploy.yml
index 3f4062b2..016fcd6a 100644
--- a/tests/installed/destructive/staged-deploy.yml
+++ b/tests/installed/destructive/staged-deploy.yml
@@ -4,7 +4,7 @@
 - name: Write staged-deploy commit
   shell: |
     ostree --repo=/ostree/repo commit --parent="${commit}" -b staged-deploy --tree=ref="${commit}" --no-bindings
-    ostree admin deploy --stage --karg-proc-cmdline --karg=ostreetest=yes staged-deploy
+    ostree admin deploy --stage staged-deploy
     test -f /run/ostree/staged-deployment
   environment:
     commit: "{{ rpmostree_status['deployments'][0]['checksum'] }}"
@@ -13,8 +13,6 @@
   shell: |
     # Assert that the previous boot had a journal entry for it
     journalctl -b "-1" -u ostree-finalize-staged.service | grep -q -e 'Transaction complete'
-    # And that we have the new kernel argument
-    grep -q -e 'ostreetest=yes' /proc/cmdline
     # And there should not be a staged deployment
     test '!' -f /run/ostree/staged-deployment
 - name: Rollback
@@ -22,14 +20,11 @@
 - include_tasks: ../tasks/reboot.yml
 - shell: |
     rpm-ostree cleanup -rp
-# And now we shouldn't have the kernel commandline entry
-- name: Check we do not have new kernel cmdline entry
-  shell: grep -qv -e 'ostreetest=yes' /proc/cmdline
 
 # Ensure we can unstage
 - name: Write staged-deploy commit, then unstage
   shell: |
-    ostree admin deploy --stage --karg-proc-cmdline --karg=ostreetest=yes staged-deploy
+    ostree admin deploy --stage staged-deploy
     ostree admin status > status.txt
     grep -qFe '(staged)' status.txt
     test -f /run/ostree/staged-deployment
@@ -43,10 +38,10 @@
 # Staged should be overwritten by non-staged
 - name: Write staged-deploy commit, then unstage
   shell: |
-    ostree admin deploy --stage --karg-proc-cmdline --karg=ostreetest=yes staged-deploy
+    ostree admin deploy --stage staged-deploy
     test -f /run/ostree/staged-deployment
     ostree --repo=/ostree/repo commit --parent="${commit}" -b nonstaged-deploy --tree=ref="${commit}" --no-bindings
-    ostree admin deploy --karg-proc-cmdline --karg=ostreetest=yes nonstaged-deploy
+    ostree admin deploy nonstaged-deploy
     ostree admin status > status.txt
     grep -vqFe '(staged)' status.txt
     test '!' -f /run/ostree/staged-deployment