keremet/rpm-ostree
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

libpriv/kargs: Strengthen and simplify new kargs APIs

Browse Source 
Note this patch only touches the *new* APIs that aren't part of
libostree.

Now that we can use `g_ptr_array_find_with_equal_func`, we can drop our
custom `_ostree_ptr_array_find`.

Also strengthen our handling of values everywhere to handle the `NULL`
case and properly support `KEYWORD` args. I ended up getting rid of
`_ostree_kernel_arg_query_status` in the process since it made that
assumption a lot and overall added more complexity than necessary.

Closes: #1796
Approved by: cgwalters
This commit is contained in:
Jonathan Lebon 2019-03-22 11:49:59 -04:00 committed by Atomic Bot
parent 96130810a5
commit 02b25c616d
4 changed files with 111 additions and 202 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

225
src/libpriv/rpmostree-kargs-process.c
View File

@ -91,40 +91,11 @@ _ostree_kernel_args_cleanup (void *loc)
} }
/* static gboolean
* Note: this function is newly added to the API strcmp0_equal (gconstpointer v1,
* gconstpointer v2)
* _ostree_ptr_array_find
* @array: a GPtrArray instance
* @val: a string value
* @out_index: the returned index
*
* Note: This is a temp replacement for 'g_ptr_array_find_with_equal_func'
* since that function was recently introduced in Glib (version 2.54),
* the version is still not updated upstream yet, thus tempoarily using
* this as a replacement.
*
* Returns: False if can not find the string value in the array
*
**/
gboolean
_ostree_ptr_array_find (GPtrArray *array,
const char *val,
int *out_index)
{ {
if (out_index == NULL) return g_strcmp0 (v1, v2) == 0;
return FALSE;
for (int counter = 0; counter < array->len; counter++)
{
const char *temp_val = array->pdata[counter];
if (g_str_equal (val, temp_val))
{
*out_index = counter;
return TRUE;
}
}
*out_index = 0; /* default to zero if not found */
return FALSE;
} }
/* Note: this function is newly added to the API */ /* Note: this function is newly added to the API */
@ -146,98 +117,6 @@ _ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs)
return NULL; return NULL;
} }
/*
* Note: this function is newly added to the API
*
* _ostree_query_arg_status:
* @kargs: A OstreeKernelArg instance
* @arg: a string to 'query status' (find its validity)
* @out_query_flag: a OstreeKernelArgQueryFlag, used to tell the caller result of finding
* @is_replaced: tell if the caller is from replace or delete
* @out_index: if successfully found, return the arg's index
* @error: an error instance
*
* This function provides check for the argument string
* to see if it is valid for deletion/replacement. More
* detailed explanation can be found in delete/replace section.
*
* Returns: False if an error is set
*/
static gboolean
_ostree_kernel_arg_query_status (OstreeKernelArgs *kargs,
const char *arg,
OstreeKernelArgQueryFlag *out_query_flag,
gboolean is_replaced,
int *out_index,
GError **error)
{
g_autofree char *arg_owned = g_strdup (arg);
g_autofree char *val = g_strdup (split_keyeq (arg_owned));
/* For replaced, it is a special case, we allow
* key=value=new_value, thus, this split is to
* discard the new value if there is one */
const char *replaced_val = split_keyeq (val);
const gboolean key_only = !*val;
GPtrArray *values = g_hash_table_lookup (kargs->table, arg_owned);
if (!values)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
"Failed to find kernel argument '%s'", arg_owned);
return FALSE;
}
const gboolean single_value = values->len == 1;
/* See if the value is inside the value list */
if (!_ostree_ptr_array_find (values, val, out_index))
{
/* Both replace and delete support empty value handlation
* thus adding a condition here to handle it separately */
if (key_only && single_value)
{
*out_query_flag = OSTREE_KERNEL_ARG_KEY_ONE_VALUE;
return TRUE;
}
/* This is a special case for replacement where
* 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 (is_replaced && single_value && !*replaced_val)
{
*out_query_flag = OSTREE_KERNEL_ARG_REPLACE_NO_SECOND_SPLIT;
return TRUE;
}
/* Handle both no value case, and the case when inputting
key=value for a replacement */
else if ((key_only || (is_replaced && !*replaced_val)) &&
!single_value)
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
"Unable to %s argument '%s' with multiple values",
is_replaced ? "replace" : "delete", arg_owned);
return FALSE;
}
else
{
g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED,
"There is no %s value for key %s",
val, arg_owned);
return FALSE;
}
}
/* We set the flag based on values len, used by replace or delete for better case handling */
if (single_value)
*out_query_flag = OSTREE_KERNEL_ARG_KEY_ONE_VALUE;
else
*out_query_flag = OSTREE_KERNEL_ARG_FOUND_KEY_MULTI_VALUE;
return TRUE;
}
/* /*
* Note: this function is newly added to the API * Note: this function is newly added to the API
* *
@ -269,31 +148,37 @@ _ostree_kernel_args_new_replace (OstreeKernelArgs *kargs,
const char *arg, const char *arg,
GError **error) GError **error)
{ {
OstreeKernelArgQueryFlag query_flag = 0;
int value_index;
if (!_ostree_kernel_arg_query_status (kargs, arg, &query_flag,
TRUE, &value_index, error))
return FALSE;
g_autofree char *arg_owned = g_strdup (arg); g_autofree char *arg_owned = g_strdup (arg);
g_autofree char *old_val = g_strdup (split_keyeq (arg_owned)); const char *key = arg_owned;
const char *possible_new_val = (query_flag == OSTREE_KERNEL_ARG_REPLACE_NO_SECOND_SPLIT) ? old_val : split_keyeq (old_val); const char *val = split_keyeq (arg_owned);
/* Similar to the delete operations, we verified in the function GPtrArray *values = g_hash_table_lookup (kargs->table, key);
* earlier that the arguments are valid, thus no check if (!values)
* performed here */ return glnx_throw (error, "No key '%s' found", key);
GPtrArray *values = g_hash_table_lookup (kargs->table, arg_owned); g_assert_cmpuint (values->len, >, 0);
/* We find the old one, we free the old memory, /* first handle the case where the user just wants to replace an old value */
* then put new one back in */ if (val && strchr (val, '='))
char *old_element = (char *)g_ptr_array_index (values, value_index); {
g_free (g_steal_pointer (&old_element)); g_autofree char *old_val = g_strdup (val);
const char *new_val = split_keyeq (old_val);
g_assert (new_val);
/* Then we assign the index to the new value */ guint i = 0;
g_ptr_array_index (values, value_index) = g_strdup (possible_new_val); if (!g_ptr_array_find_with_equal_func (values, old_val, strcmp0_equal, &i))
return glnx_throw (error, "No karg '%s=%s' found", key, old_val);
g_clear_pointer (&values->pdata[i], g_free);
values->pdata[i] = g_strdup (new_val);
return TRUE;
}
/* can't know which val to replace without the old_val=new_val syntax */
if (values->len > 1)
return glnx_throw (error, "Multiple values for key '%s' found", key);
g_clear_pointer (&values->pdata[0], g_free);
values->pdata[0] = g_strdup (val);
return TRUE; return TRUE;
} }
@ -323,41 +208,45 @@ _ostree_kernel_args_new_replace (OstreeKernelArgs *kargs,
* *
**/ **/
gboolean gboolean
_ostree_kernel_args_delete (OstreeKernelArgs *kargs, _ostree_kernel_args_delete (OstreeKernelArgs *kargs,
const char *arg, const char *arg,
GError **error) GError **error)
{ {
OstreeKernelArgQueryFlag query_flag = 0;
int value_index;
if (!_ostree_kernel_arg_query_status (kargs, arg, &query_flag,
FALSE, &value_index, error))
return FALSE;
/* We then know the arg can be found and is valid currently */
g_autofree char *arg_owned = g_strdup (arg); g_autofree char *arg_owned = g_strdup (arg);
split_keyeq (arg_owned); const char *key = arg_owned;
const char *val = split_keyeq (arg_owned);
GPtrArray *values = g_hash_table_lookup (kargs->table, arg_owned); GPtrArray *values = g_hash_table_lookup (kargs->table, key);
if (!values)
return glnx_throw (error, "No key '%s' found", key);
g_assert_cmpuint (values->len, >, 0);
/* This tells us to delete that key directly */ /* special-case: we allow deleting by key only if there's only one val */
if (query_flag == OSTREE_KERNEL_ARG_KEY_ONE_VALUE) if (values->len == 1)
return _ostree_kernel_args_delete_key_entry (kargs, arg_owned, error);
else if (query_flag == OSTREE_KERNEL_ARG_FOUND_KEY_MULTI_VALUE)
{ {
g_ptr_array_remove_index (values, value_index); /* but if a specific val was passed, check that it's the same */
return TRUE; if (val && !strcmp0_equal (val, values->pdata[0]))
return glnx_throw (error, "No karg '%s=%s' found", key, val);
return _ostree_kernel_args_delete_key_entry (kargs, key, error);
} }
else
g_assert_not_reached();
/* multiple values, but just key supplied? error out */
if (!val)
return glnx_throw (error, "Multiple values for key '%s' found", key);
guint i = 0;
if (!g_ptr_array_find_with_equal_func (values, val, strcmp0_equal, &i))
return glnx_throw (error, "No karg '%s' found", arg);
g_ptr_array_remove_index (values, i);
return TRUE;
} }
/* /*
* Note: this is a new function added to the API * Note: this is a new function added to the API
* *
* _ostree_kernel_args_delete_key_entry * _ostree_kernel_args_delete_key_entry
* @kargs: an OstreeKernelArgs intanc * @kargs: an OstreeKernelArgs instance
* @key: the key to remove * @key: the key to remove
* @error: an GError instance * @error: an GError instance
* *
@ -385,8 +274,8 @@ _ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs,
} }
/* Then remove the key from order table */ /* Then remove the key from order table */
int key_index; guint key_index;
g_assert (_ostree_ptr_array_find (kargs->order, key, &key_index)); g_assert (g_ptr_array_find_with_equal_func (kargs->order, key, g_str_equal, &key_index));
g_assert (g_ptr_array_remove_index (kargs->order, key_index)); g_assert (g_ptr_array_remove_index (kargs->order, key_index));
return TRUE; return TRUE;
} }

15
src/libpriv/rpmostree-kargs-process.h
View File

@ -25,21 +25,6 @@ G_BEGIN_DECLS
typedef struct _OstreeKernelArgs OstreeKernelArgs; typedef struct _OstreeKernelArgs OstreeKernelArgs;
/*
* Note: this is newly added to the API:
* This flag is used to track the 'validity' and status of the arguments
*
* OSTREE_KERNEL_ARG_KEY_ONE_VALUE: means there is only one value associated with the key
* OSTREE_KERNEL_ARG_REPLACE_NO_SECOND_SPLIT: means to tell 'replace function', the arg only need to be split once
* OSTREE_KERNEL_ARG_FOUND_KEY_MULTI_VALUE: means the key found has multiple values associated with it
*
**/
typedef enum {
OSTREE_KERNEL_ARG_KEY_ONE_VALUE = (1 << 0),
OSTREE_KERNEL_ARG_FOUND_KEY_MULTI_VALUE = (1 << 1),
OSTREE_KERNEL_ARG_REPLACE_NO_SECOND_SPLIT = (1 << 2),
} OstreeKernelArgQueryFlag;
GHashTable* _ostree_kernel_arg_get_kargs_table (OstreeKernelArgs *kargs); GHashTable* _ostree_kernel_arg_get_kargs_table (OstreeKernelArgs *kargs);
GPtrArray* _ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs); GPtrArray* _ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs);

69
tests/check/test-kargs.c
View File

@ -16,12 +16,13 @@ test_kargs_delete (void)
{ {
g_autoptr(GError) error = NULL; g_autoptr(GError) error = NULL;
gboolean ret; gboolean ret;
int key_index = 0;
__attribute__((cleanup(_ostree_kernel_args_cleanup))) OstreeKernelArgs *karg = _ostree_kernel_args_new (); __attribute__((cleanup(_ostree_kernel_args_cleanup))) OstreeKernelArgs *karg = _ostree_kernel_args_new ();
_ostree_kernel_args_append (karg, "single_key=test"); _ostree_kernel_args_append (karg, "single_key=test");
_ostree_kernel_args_append (karg, "test=firstval"); _ostree_kernel_args_append (karg, "test=firstval");
_ostree_kernel_args_append (karg, "test=secondval"); _ostree_kernel_args_append (karg, "test=secondval");
_ostree_kernel_args_append (karg, "test=");
_ostree_kernel_args_append (karg, "test");
/* Delete a non-existant key should fail */ /* Delete a non-existant key should fail */
ret = _ostree_kernel_args_delete (karg, "non_existant_key", &error); ret = _ostree_kernel_args_delete (karg, "non_existant_key", &error);
@ -41,21 +42,43 @@ test_kargs_delete (void)
g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED); g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
g_clear_error (&error); g_clear_error (&error);
/* Delete a key with only one value should fail if the value doesn't match */
ret = _ostree_kernel_args_delete (karg, "single_key=non_existent_value", &error);
g_assert (!ret);
g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
g_clear_error (&error);
/* Delete a key with only one value should succeed by only specifying key */ /* Delete a key with only one value should succeed by only specifying key */
ret = _ostree_kernel_args_delete (karg, "single_key", &error); ret = _ostree_kernel_args_delete (karg, "single_key", &error);
g_assert_no_error (error); g_assert_no_error (error);
g_assert (ret); g_assert (ret);
/* verify the value array is properly updated */ /* verify the value array is properly updated */
GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (karg); GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (karg);
g_assert (!_ostree_ptr_array_find (kargs_array, "single_key", &key_index)); g_assert (!g_ptr_array_find_with_equal_func (kargs_array, "single_key", g_str_equal, NULL));
g_assert (!check_string_existance (karg, "single_key")); g_assert (!check_string_existance (karg, "single_key"));
/* Delete a key/value pair by specifying correct key=value should succeed */ /* Delete specific key/value pair */
ret = _ostree_kernel_args_delete (karg, "test=secondval", &error); ret = _ostree_kernel_args_delete (karg, "test=secondval", &error);
g_assert_no_error (error); g_assert_no_error (error);
g_assert (ret); g_assert (ret);
g_assert (!check_string_existance (karg, "test=secondval")); g_assert (!check_string_existance (karg, "test=secondval"));
/* Delete key/value pair with empty string value */
ret = _ostree_kernel_args_delete (karg, "test=", &error);
g_assert_no_error (error);
g_assert (ret);
g_assert (!check_string_existance (karg, "test="));
ret = _ostree_kernel_args_delete (karg, "test=firstval", &error);
g_assert_no_error (error);
g_assert (ret);
g_assert (!check_string_existance (karg, "test=firstval"));
/* And now we should be able to delete the last key */
ret = _ostree_kernel_args_delete (karg, "test", &error);
g_assert_no_error (error);
g_assert (ret);
g_assert (!check_string_existance (karg, "test"));
} }
static void static void
@ -93,6 +116,7 @@ test_kargs_replace (void)
ret = _ostree_kernel_args_new_replace (karg, "single_key=newvalue", &error); ret = _ostree_kernel_args_new_replace (karg, "single_key=newvalue", &error);
g_assert_no_error (error); g_assert_no_error (error);
g_assert (ret); g_assert (ret);
g_assert (!check_string_existance (karg, "single_key"));
g_assert (check_string_existance (karg, "single_key=newvalue")); g_assert (check_string_existance (karg, "single_key=newvalue"));
/* Replace with input key=value=newvalue if key and value both /* Replace with input key=value=newvalue if key and value both
@ -101,9 +125,16 @@ test_kargs_replace (void)
ret = _ostree_kernel_args_new_replace (karg, "test=firstval=newval", &error); ret = _ostree_kernel_args_new_replace (karg, "test=firstval=newval", &error);
g_assert_no_error (error); g_assert_no_error (error);
g_assert (ret); g_assert (ret);
g_assert (!check_string_existance (karg, "test=firstval"));
g_assert (check_string_existance (karg, "test=newval")); g_assert (check_string_existance (karg, "test=newval"));
} }
static gboolean
strcmp0_equal (gconstpointer v1,
gconstpointer v2)
{
return g_strcmp0 (v1, v2) == 0;
}
/* In this function, we want to verify that _ostree_kernel_args_append /* In this function, we want to verify that _ostree_kernel_args_append
* and _ostree_kernel_args_to_string is correct. After that * and _ostree_kernel_args_to_string is correct. After that
@ -113,16 +144,12 @@ static void
test_kargs_append (void) test_kargs_append (void)
{ {
/* Note, here we assume the _ostree_ptr_array_find has
* a working functionality, as it shares a similar logic
* as the GLib function, and easy to tell the correctness of it
*/
int test_num = 0;
__attribute__((cleanup(_ostree_kernel_args_cleanup))) OstreeKernelArgs *append_arg = _ostree_kernel_args_new (); __attribute__((cleanup(_ostree_kernel_args_cleanup))) OstreeKernelArgs *append_arg = _ostree_kernel_args_new ();
/* Some valid cases (key=value) pair */ /* Some valid cases (key=value) pair */
_ostree_kernel_args_append (append_arg, "test=valid"); _ostree_kernel_args_append (append_arg, "test=valid");
_ostree_kernel_args_append (append_arg, "test=secondvalid"); _ostree_kernel_args_append (append_arg, "test=secondvalid");
/* Add some 'invalid cases' */ _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");
/* We loops through the kargs inside table to verify /* We loops through the kargs inside table to verify
@ -132,18 +159,24 @@ test_kargs_append (void)
GHashTable *kargs_table = _ostree_kernel_arg_get_kargs_table (append_arg); GHashTable *kargs_table = _ostree_kernel_arg_get_kargs_table (append_arg);
GLNX_HASH_TABLE_FOREACH_KV (kargs_table, const char*, key, GPtrArray*, value_array) GLNX_HASH_TABLE_FOREACH_KV (kargs_table, const char*, key, GPtrArray*, value_array)
{ {
if ( g_strcmp0 (key, "test") == 0) if (g_str_equal (key, "test"))
{ {
g_assert (_ostree_ptr_array_find (value_array, "valid", &test_num)); g_assert (g_ptr_array_find_with_equal_func (value_array, "valid", strcmp0_equal, NULL));
g_assert (_ostree_ptr_array_find (value_array, "secondvalid", &test_num)); g_assert (g_ptr_array_find_with_equal_func (value_array, "secondvalid", strcmp0_equal, NULL));
g_assert (g_ptr_array_find_with_equal_func (value_array, "", strcmp0_equal, NULL));
g_assert (g_ptr_array_find_with_equal_func (value_array, NULL, strcmp0_equal, NULL));
} }
else else
g_assert (_ostree_ptr_array_find (value_array, "", &test_num)); {
g_assert_cmpstr (key, ==, "second_test");
g_assert (g_ptr_array_find_with_equal_func (value_array, NULL, strcmp0_equal, NULL));
}
} }
/* verify the value array is properly updated */ /* verify the value array is properly updated */
GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (append_arg); GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (append_arg);
g_assert (_ostree_ptr_array_find (kargs_array, "test", &test_num)); g_assert (g_ptr_array_find_with_equal_func (kargs_array, "test", g_str_equal, NULL));
g_assert (_ostree_ptr_array_find (kargs_array, "second_test", &test_num)); g_assert (g_ptr_array_find_with_equal_func (kargs_array, "second_test", g_str_equal, NULL));
/* Up till this point, we verified that the above was all correct, we then /* Up till this point, we verified that the above was all correct, we then
* check _ostree_kernel_args_to_string has the right result * check _ostree_kernel_args_to_string has the right result
@ -151,9 +184,11 @@ test_kargs_append (void)
g_autofree gchar* kargs_str = _ostree_kernel_args_to_string (append_arg); 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 = g_strsplit(kargs_str, " ", -1);
g_assert (g_strv_contains ((const char* const *)kargs_list, "test=valid")); g_assert (g_strv_contains ((const char* const *)kargs_list, "test=valid"));
g_assert (g_strv_contains ((const char* const *)kargs_list, "second_test"));
g_assert (g_strv_contains ((const char* const *)kargs_list, "test=secondvalid")); g_assert (g_strv_contains ((const char* const *)kargs_list, "test=secondvalid"));
g_assert_cmpint (3, ==, g_strv_length (kargs_list)); 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));
} }

4
tests/vmcheck/test-kernel-args.sh
View File

@ -57,7 +57,7 @@ echo "ok delete a single key/value pair"
if vm_rpmostree kargs --delete APPENDARG 2>err.txt; then if vm_rpmostree kargs --delete APPENDARG 2>err.txt; then
assert_not_reached "Delete A key with multiple values unexpectedly succeeded" assert_not_reached "Delete A key with multiple values unexpectedly succeeded"
fi fi
assert_file_has_content err.txt "Unable to delete argument 'APPENDARG' with multiple values" assert_file_has_content err.txt "Multiple values for key 'APPENDARG' found"
echo "ok failed to delete key with multiple values" echo "ok failed to delete key with multiple values"
vm_kargs_now --delete APPENDARG=VALAPPEND vm_kargs_now --delete APPENDARG=VALAPPEND
@ -82,7 +82,7 @@ echo "ok replacing one key/value pair"
if vm_rpmostree kargs --replace=REPLACE_MULTI_TEST=ERR 2>err.txt; then if vm_rpmostree kargs --replace=REPLACE_MULTI_TEST=ERR 2>err.txt; then
assert_not_reached "Replace a key with multiple values unexpectedly succeeded" assert_not_reached "Replace a key with multiple values unexpectedly succeeded"
fi fi
assert_file_has_content err.txt "Unable to replace argument 'REPLACE_MULTI_TEST' with multiple values" assert_file_has_content err.txt "Multiple values for key 'REPLACE_MULTI_TEST' found"
echo "ok failed to replace key with multiple values" echo "ok failed to replace key with multiple values"
# Test for replacing one of the values for multi value keys # Test for replacing one of the values for multi value keys