Skip to content

Commit

Permalink
kargs: parse spaces in kargs input and keep quotes
Browse files Browse the repository at this point in the history 
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 coreos/rpm-ostree#4821
  • Loading branch information
@HuijingHei
HuijingHei committed Mar 8, 2024
1 parent d95c2f8 commit abc7d5b
Show file tree
Hide file tree
Showing 4 changed files with 169 additions and 79 deletions.
199 changes: 130 additions & 69 deletions src/libostree/ostree-kernel-args.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
*
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}
}

/**
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 1 addition & 4 deletions src/ostree/ot-admin-builtin-deploy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
9 changes: 8 additions & 1 deletion tests/test-admin-deploy-karg.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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"
35 changes: 30 additions & 5 deletions tests/test-kargs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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));
}
}

Expand All @@ -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
Expand Down

0 comments on commit abc7d5b

Please sign in to comment.