kargs: parse spaces in kargs input and keep quotes

According to Jonathan's suggestion, should fix the code from
ostree repo.

With this patch:
- kargs input like "init_on_alloc=1 init_on_free=1", will be
parsed as 2 seperated args `init_on_alloc=1` and `init_on_free=1`,
instead of whole;
- According to https://www.kernel.org/doc/html/v4.14/admin-guide/kernel-parameters.html,
need to keep spaces in double-quotes, like `param="spaces in here"`
will be parsed as whole instead of 3.

Fixes https://github.com/coreos/rpm-ostree/issues/4821

diff --git a/src/libostree/ostree-kernel-args.c b/src/libostree/ostree-kernel-args.c
index 9a9e0be24878181ffc5c0c484d1ed9f1056f44f3..57357a30f8e93459bad415d46245ee8e1eee2701 100644 (file)
--- a/src/libostree/ostree-kernel-args.c
+++ b/src/libostree/ostree-kernel-args.c
@@ -163,6 +163,44 @@ strcmp0_equal (gconstpointer v1, gconstpointer v2)
   return g_strcmp0 (v1, v2) == 0;
 }
 
+/* Split string with spaces, but keep quotes.
+   For example, "test=\"1 2\"" will be saved as whole.
+ */
+static char **
+split_kernel_args (const char *str)
+{
+  gboolean quoted = FALSE;
+  g_return_val_if_fail (str != NULL, NULL);
+
+  GPtrArray *strv = g_ptr_array_new ();
+
+  size_t len = strlen (str);
+  // Skip first spaces
+  const char *start = str + strspn (str, " ");
+  for (const char *iter = start; iter && *iter; iter++)
+    {
+      if (*iter == '"')
+        quoted = !quoted;
+      else if (*iter == ' ' && !quoted)
+        {
+          g_ptr_array_add (strv, g_strndup (start, iter - start));
+          start = iter + 1;
+        }
+    }
+
+  // Add the last slice
+  if (!quoted)
+    g_ptr_array_add (strv, g_strndup (start, str + len - start));
+  else
+    {
+      g_debug ("Missing terminating quote in '%s'.\n", str);
+      g_assert_false (quoted);
+    }
+
+  g_ptr_array_add (strv, NULL);
+  return (char **)g_ptr_array_free (strv, FALSE);
+}
+
 /**
  * ostree_kernel_args_new: (skip)
  *
@@ -285,35 +323,42 @@ _ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs)
 gboolean
 ostree_kernel_args_new_replace (OstreeKernelArgs *kargs, const char *arg, GError **error)
 {
-  g_autofree char *arg_owned = g_strdup (arg);
-  const char *key = arg_owned;
-  const char *val = split_keyeq (arg_owned);
-
-  GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
-  if (!entries)
-    return glnx_throw (error, "No key '%s' found", key);
-  g_assert_cmpuint (entries->len, >, 0);
+  // Split the arg
+  g_auto (GStrv) argv = split_kernel_args (arg);
 
-  /* first handle the case where the user just wants to replace an old value */
-  if (val && strchr (val, '='))
+  for (char **iter = argv; iter && *iter; iter++)
     {
-      g_autofree char *old_val = g_strdup (val);
-      const char *new_val = split_keyeq (old_val);
-      g_assert (new_val);
+      g_autofree char *arg_owned = g_strdup (*iter);
+      const char *key = arg_owned;
+      const char *val = split_keyeq (arg_owned);
 
-      guint i = 0;
-      if (!ot_ptr_array_find_with_equal_func (entries, old_val, kernel_args_entry_value_equal, &i))
-        return glnx_throw (error, "No karg '%s=%s' found", key, old_val);
+      GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
+      if (!entries)
+        return glnx_throw (error, "No key '%s' found", key);
+      g_assert_cmpuint (entries->len, >, 0);
 
-      kernel_args_entry_replace_value (entries->pdata[i], new_val);
-      return TRUE;
-    }
+      /* first handle the case where the user just wants to replace an old value */
+      if (val && strchr (val, '='))
+        {
+          g_autofree char *old_val = g_strdup (val);
+          const char *new_val = split_keyeq (old_val);
+          g_assert (new_val);
+
+          guint i = 0;
+          if (!ot_ptr_array_find_with_equal_func (entries, old_val, kernel_args_entry_value_equal,
+                                                  &i))
+            return glnx_throw (error, "No karg '%s=%s' found", key, old_val);
+
+          kernel_args_entry_replace_value (entries->pdata[i], new_val);
+          continue;
+        }
 
-  /* can't know which val to replace without the old_val=new_val syntax */
-  if (entries->len > 1)
-    return glnx_throw (error, "Multiple values for key '%s' found", key);
+      /* can't know which val to replace without the old_val=new_val syntax */
+      if (entries->len > 1)
+        return glnx_throw (error, "Multiple values for key '%s' found", key);
 
-  kernel_args_entry_replace_value (entries->pdata[0], val);
+      kernel_args_entry_replace_value (entries->pdata[0], val);
+    }
   return TRUE;
 }
 
@@ -381,39 +426,48 @@ ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs, const char *key, G
 gboolean
 ostree_kernel_args_delete (OstreeKernelArgs *kargs, const char *arg, GError **error)
 {
-  g_autofree char *arg_owned = g_strdup (arg);
-  const char *key = arg_owned;
-  const char *val = split_keyeq (arg_owned);
-
-  GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
-  if (!entries)
-    return glnx_throw (error, "No key '%s' found", key);
-  g_assert_cmpuint (entries->len, >, 0);
+  // Split the arg
+  g_auto (GStrv) argv = split_kernel_args (arg);
 
-  /* special-case: we allow deleting by key only if there's only one val */
-  if (entries->len == 1)
+  for (char **iter = argv; iter && *iter; iter++)
     {
-      /* but if a specific val was passed, check that it's the same */
-      OstreeKernelArgsEntry *e = entries->pdata[0];
-      if (val && !strcmp0_equal (val, _ostree_kernel_args_entry_get_value (e)))
-        return glnx_throw (error, "No karg '%s=%s' found", key, val);
-      return ostree_kernel_args_delete_key_entry (kargs, key, error);
-    }
+      g_autofree char *arg_owned = g_strdup (*iter);
 
-  /* note val might be NULL here, in which case we're looking for `key`, not `key=` or
-   * `key=val` */
-  guint i = 0;
-  if (!ot_ptr_array_find_with_equal_func (entries, val, kernel_args_entry_value_equal, &i))
-    {
-      if (!val)
-        /* didn't find NULL -> only key= key=val1 key=val2 style things left, so the user
-         * needs to be more specific */
-        return glnx_throw (error, "Multiple values for key '%s' found", arg);
-      return glnx_throw (error, "No karg '%s' found", arg);
-    }
+      const char *key = arg_owned;
+      const char *val = split_keyeq (arg_owned);
+
+      GPtrArray *entries = g_hash_table_lookup (kargs->table, key);
+      if (!entries)
+        return glnx_throw (error, "No key '%s' found", key);
+      g_assert_cmpuint (entries->len, >, 0);
+
+      /* special-case: we allow deleting by key only if there's only one val */
+      if (entries->len == 1)
+        {
+          /* but if a specific val was passed, check that it's the same */
+          OstreeKernelArgsEntry *e = entries->pdata[0];
+          if (val && !strcmp0_equal (val, _ostree_kernel_args_entry_get_value (e)))
+            return glnx_throw (error, "No karg '%s=%s' found", key, val);
+          if (!ostree_kernel_args_delete_key_entry (kargs, key, error))
+            return glnx_throw (error, "Remove key entry '%s=%s' failed.", key, val);
+          continue;
+        }
 
-  g_assert (g_ptr_array_remove (kargs->order, entries->pdata[i]));
-  g_assert (g_ptr_array_remove_index (entries, i));
+      /* note val might be NULL here, in which case we're looking for `key`, not `key=` or
+       * `key=val` */
+      guint i = 0;
+      if (!ot_ptr_array_find_with_equal_func (entries, val, kernel_args_entry_value_equal, &i))
+        {
+          if (!val)
+            /* didn't find NULL -> only key= key=val1 key=val2 style things left, so the user
+             * needs to be more specific */
+            return glnx_throw (error, "Multiple values for key '%s' found", arg);
+          return glnx_throw (error, "No karg '%s' found", arg);
+        }
+
+      g_assert (g_ptr_array_remove (kargs->order, entries->pdata[i]));
+      g_assert (g_ptr_array_remove_index (entries, i));
+    }
   return TRUE;
 }
 
@@ -499,27 +553,34 @@ ostree_kernel_args_replace (OstreeKernelArgs *kargs, const char *arg)
 void
 ostree_kernel_args_append (OstreeKernelArgs *kargs, const char *arg)
 {
-  gboolean existed = TRUE;
-  GPtrArray *entries = NULL;
-  char *duped = g_strdup (arg);
-  const char *val = split_keyeq (duped);
+  // Split the arg
+  g_auto (GStrv) argv = split_kernel_args (arg);
 
-  entries = g_hash_table_lookup (kargs->table, duped);
-  if (!entries)
+  for (char **iter = argv; iter && *iter; iter++)
     {
-      entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table);
-      existed = FALSE;
-    }
+      gboolean existed = TRUE;
+      GPtrArray *entries = NULL;
 
-  OstreeKernelArgsEntry *entry = _ostree_kernel_args_entry_new ();
-  _ostree_kernel_args_entry_set_key (entry, duped);
-  _ostree_kernel_args_entry_set_value (entry, g_strdup (val));
+      char *duped = g_strdup (*iter);
+      const char *val = split_keyeq (duped);
 
-  g_ptr_array_add (entries, entry);
-  g_ptr_array_add (kargs->order, entry);
+      entries = g_hash_table_lookup (kargs->table, duped);
+      if (!entries)
+        {
+          entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table);
+          existed = FALSE;
+        }
+
+      OstreeKernelArgsEntry *entry = _ostree_kernel_args_entry_new ();
+      _ostree_kernel_args_entry_set_key (entry, duped);
+      _ostree_kernel_args_entry_set_value (entry, g_strdup (val));
 
-  if (!existed)
-    g_hash_table_replace (kargs->table, duped, entries);
+      g_ptr_array_add (entries, entry);
+      g_ptr_array_add (kargs->order, entry);
+
+      if (!existed)
+        g_hash_table_replace (kargs->table, duped, entries);
+    }
 }
 
 /**
@@ -644,7 +705,7 @@ ostree_kernel_args_parse_append (OstreeKernelArgs *kargs, const char *options)
   if (!options)
     return;
 
-  args = g_strsplit (options, " ", -1);
+  args = split_kernel_args (options);
   for (iter = args; *iter; iter++)
     {
       char *arg = *iter;

diff --git a/src/ostree/ot-admin-builtin-deploy.c b/src/ostree/ot-admin-builtin-deploy.c
index f14cce5d54bd046ed028bb6ad0e383ea05df7b2c..66d857241bdb747ec74572c0f9d9b5e8bedd0c4b 100644 (file)
--- a/src/ostree/ot-admin-builtin-deploy.c
+++ b/src/ostree/ot-admin-builtin-deploy.c
@@ -184,10 +184,7 @@ ot_admin_builtin_deploy (int argc, char **argv, OstreeCommandInvocation *invocat
            && (opt_kernel_argv || opt_kernel_argv_append || opt_kernel_argv_delete))
     {
       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);
+      kargs = ostree_kernel_args_from_string (ostree_bootconfig_parser_get (bootconfig, "options"));
     }
 
   /* Now replace/extend the above set.  Note that if no options are specified,

diff --git a/tests/test-admin-deploy-karg.sh b/tests/test-admin-deploy-karg.sh
index b47bc07168d4b0993c82175058a36469a7980608..89d613f8badada20c2676a692393fc9e911bfc21 100755 (executable)
--- a/tests/test-admin-deploy-karg.sh
+++ b/tests/test-admin-deploy-karg.sh
@@ -24,7 +24,7 @@ set -euo pipefail
 # Exports OSTREE_SYSROOT so --sysroot not needed.
 setup_os_repository "archive" "syslinux"
 
-echo "1..4"
+echo "1..5"
 
 ${CMD_PREFIX} ostree --repo=sysroot/ostree/repo pull-local --remote=testos testos-repo testos/buildmain/x86_64-runtime
 rev=$(${CMD_PREFIX} ostree --repo=sysroot/ostree/repo rev-parse testos/buildmain/x86_64-runtime)
@@ -77,3 +77,10 @@ assert_not_file_has_content sysroot/boot/loader/entries/ostree-2.conf 'options.*
 assert_not_file_has_content sysroot/boot/loader/entries/ostree-2.conf 'options.*APPENDARG=VALAPPEND'
 
 echo "ok deploy --karg-delete"
+
+${CMD_PREFIX} ostree admin deploy --os=testos --karg-append 'test="1 2"' testos:testos/buildmain/x86_64-runtime
+assert_file_has_content sysroot/boot/loader/entries/ostree-2.conf 'options.*test="1 2"'
+${CMD_PREFIX} ostree admin deploy --os=testos --karg-delete 'test="1 2"' testos:testos/buildmain/x86_64-runtime
+assert_not_file_has_content sysroot/boot/loader/entries/ostree-2.conf 'options.*test="1 2"'
+
+echo "ok deploy --karg-delete with quotes"

diff --git a/tests/test-kargs.c b/tests/test-kargs.c
index bd1735d4b8af03e8577742730a07a405992cf4dc..90f11e5f85597a53e4896c36be9eff1d8c200535 100644 (file)
--- a/tests/test-kargs.c
+++ b/tests/test-kargs.c
@@ -24,8 +24,7 @@
 static gboolean
 check_string_existance (OstreeKernelArgs *karg, const char *string_to_find)
 {
-  g_autofree gchar *string_with_spaces = ostree_kernel_args_to_string (karg);
-  g_auto (GStrv) string_list = g_strsplit (string_with_spaces, " ", -1);
+  g_auto (GStrv) string_list = ostree_kernel_args_to_strv (karg);
   return g_strv_contains ((const char *const *)string_list, string_to_find);
 }
 
@@ -140,6 +139,16 @@ test_kargs_delete (void)
   g_assert_no_error (error);
   g_assert (ret);
   g_assert (!check_string_existance (karg, "nosmt"));
+
+  /* Make sure we also support quotes and spaces */
+  ostree_kernel_args_append (karg, "foo=\"1 2\" bar=0");
+  check_string_existance (karg, "foo=\"1 2\"");
+  check_string_existance (karg, "bar=0");
+  ret = ostree_kernel_args_delete (karg, "foo=\"1 2\" bar=0", &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+  g_assert (!check_string_existance (karg, "foo=\"1 2\""));
+  g_assert (!check_string_existance (karg, "bar=0"));
 }
 
 static void
@@ -189,6 +198,16 @@ test_kargs_replace (void)
   g_assert (ret);
   g_assert (!check_string_existance (karg, "test=firstval"));
   g_assert (check_string_existance (karg, "test=newval"));
+
+  /* Replace with input contains quotes and spaces, it should support */
+  ostree_kernel_args_append (karg, "foo=1 bar=\"1 2\"");
+  ret = ostree_kernel_args_new_replace (karg, "foo=\"1 2\" bar", &error);
+  g_assert_no_error (error);
+  g_assert (ret);
+  g_assert (!check_string_existance (karg, "foo=1"));
+  g_assert (!check_string_existance (karg, "bar=\"1 2\""));
+  g_assert (check_string_existance (karg, "foo=\"1 2\""));
+  g_assert (check_string_existance (karg, "bar"));
 }
 
 /* In this function, we want to verify that ostree_kernel_args_append
@@ -206,6 +225,7 @@ test_kargs_append (void)
   ostree_kernel_args_append (append_arg, "test=");
   ostree_kernel_args_append (append_arg, "test");
   ostree_kernel_args_append (append_arg, "second_test");
+  ostree_kernel_args_append (append_arg, "second_test=0 second_test=\"1 2\"");
 
   /* We loops through the kargs inside table to verify
    * the functionality of append because at this stage
@@ -230,6 +250,10 @@ test_kargs_append (void)
           g_assert_cmpstr (key, ==, "second_test");
           g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL,
                                                        kernel_args_entry_value_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, "0",
+                                                       kernel_args_entry_value_equal, NULL));
+          g_assert (ot_ptr_array_find_with_equal_func (value_array, "\"1 2\"",
+                                                       kernel_args_entry_value_equal, NULL));
         }
     }
 
@@ -243,14 +267,15 @@ test_kargs_append (void)
   /* Up till this point, we verified that the above was all correct, we then
    * check ostree_kernel_args_to_string has the right result
    */
-  g_autofree gchar *kargs_str = ostree_kernel_args_to_string (append_arg);
-  g_auto (GStrv) kargs_list = g_strsplit (kargs_str, " ", -1);
+  g_auto (GStrv) kargs_list = ostree_kernel_args_to_strv (append_arg);
   g_assert (g_strv_contains ((const char *const *)kargs_list, "test=valid"));
   g_assert (g_strv_contains ((const char *const *)kargs_list, "test=secondvalid"));
   g_assert (g_strv_contains ((const char *const *)kargs_list, "test="));
   g_assert (g_strv_contains ((const char *const *)kargs_list, "test"));
   g_assert (g_strv_contains ((const char *const *)kargs_list, "second_test"));
-  g_assert_cmpint (5, ==, g_strv_length (kargs_list));
+  g_assert (g_strv_contains ((const char *const *)kargs_list, "second_test=0"));
+  g_assert (g_strv_contains ((const char *const *)kargs_list, "second_test=\"1 2\""));
+  g_assert_cmpint (7, ==, g_strv_length (kargs_list));
 }
 
 int