diff --git a/src/libostree/ostree-kernel-args.c b/src/libostree/ostree-kernel-args.c index dbf2ec8a..c6300823 100644 --- a/src/libostree/ostree-kernel-args.c +++ b/src/libostree/ostree-kernel-args.c @@ -30,6 +30,111 @@ struct _OstreeKernelArgs { GHashTable *table; }; +struct _OstreeKernelArgsEntry { + char *key; + char *value; +}; + +char * +_ostree_kernel_args_entry_get_key (const OstreeKernelArgsEntry *e) +{ + return e->key; +} + +char * +_ostree_kernel_args_entry_get_value (const OstreeKernelArgsEntry *e) +{ + return e->value; +} + +void +_ostree_kernel_args_entry_set_key (OstreeKernelArgsEntry *e, + char *key) +{ + e->key = key; +} + +void +_ostree_kernel_args_entry_set_value (OstreeKernelArgsEntry *e, + char *value) +{ + e->value = value; +} + +char * +_ostree_kernel_args_get_key_index (const OstreeKernelArgs *kargs, + int i) +{ + OstreeKernelArgsEntry *e = kargs->order->pdata[i]; + return e->key; +} + +char * +_ostree_kernel_args_get_value_index (const OstreeKernelArgs *kargs, + int i) +{ + OstreeKernelArgsEntry *e = kargs->order->pdata[i]; + return e->value; +} + +OstreeKernelArgsEntry * +_ostree_kernel_args_entry_new (void) +{ + return g_new0 (OstreeKernelArgsEntry, 1); +} + +void +_ostree_kernel_args_entry_value_free (OstreeKernelArgsEntry *e) +{ + g_clear_pointer (&e->value, g_free); +} + +/* Free the value field, and the entry. This should be set as the free + * function, for all pointer arrays stored in the hash table. + */ +static void +kernel_args_entry_free_from_table (gpointer data) +{ + OstreeKernelArgsEntry *e = data; + // The hash table owns the key; do not free it here. + g_free (_ostree_kernel_args_entry_get_value (e)); + g_free (e); +} + +static gboolean +kernel_args_entry_value_equal (gconstpointer data, + gconstpointer value) +{ + const OstreeKernelArgsEntry *e = data; + return g_strcmp0 (_ostree_kernel_args_entry_get_value (e), value) == 0; +} + +static gboolean +kernel_args_entry_key_equal (gconstpointer data, + gconstpointer key) +{ + const OstreeKernelArgsEntry *e = data; + return g_strcmp0 (_ostree_kernel_args_entry_get_key (e), key) == 0; +} + +static void +kernel_args_entry_replace_value (OstreeKernelArgsEntry *e, + const char *value) +{ + g_assert (e); + _ostree_kernel_args_entry_value_free (e); + _ostree_kernel_args_entry_set_value (e, g_strdup (value)); +} + +static void +kernel_args_remove_entries_from_order (GPtrArray *order, + GPtrArray *entries) +{ + g_assert (entries); + for (int i = 0; i < entries->len; i++) + g_assert (g_ptr_array_remove (order, entries->pdata[i])); +} + static char * split_keyeq (char *arg) { @@ -82,9 +187,11 @@ ostree_kernel_args_new (void) { OstreeKernelArgs *ret; ret = g_new0 (OstreeKernelArgs, 1); - ret->order = g_ptr_array_new_with_free_func (g_free); + /* Hash table owns the kernel args entries, since it uses keys to index, + * and its values are used to locate entries in the order array. */ ret->table = g_hash_table_new_full (g_str_hash, g_str_equal, - NULL, (GDestroyNotify)g_ptr_array_unref); + g_free, (GDestroyNotify)g_ptr_array_unref); + ret->order = g_ptr_array_new_with_free_func (NULL); return ret; } @@ -194,10 +301,10 @@ ostree_kernel_args_new_replace (OstreeKernelArgs *kargs, const char *key = arg_owned; const char *val = split_keyeq (arg_owned); - GPtrArray *values = g_hash_table_lookup (kargs->table, key); - if (!values) + GPtrArray *entries = g_hash_table_lookup (kargs->table, key); + if (!entries) return glnx_throw (error, "No key '%s' found", key); - g_assert_cmpuint (values->len, >, 0); + g_assert_cmpuint (entries->len, >, 0); /* first handle the case where the user just wants to replace an old value */ if (val && strchr (val, '=')) @@ -207,20 +314,18 @@ ostree_kernel_args_new_replace (OstreeKernelArgs *kargs, g_assert (new_val); guint i = 0; - if (!ot_ptr_array_find_with_equal_func (values, old_val, strcmp0_equal, &i)) + 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); - g_clear_pointer (&values->pdata[i], g_free); - values->pdata[i] = g_strdup (new_val); + kernel_args_entry_replace_value (entries->pdata[i], new_val); return TRUE; } /* can't know which val to replace without the old_val=new_val syntax */ - if (values->len > 1) + if (entries->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); + kernel_args_entry_replace_value (entries->pdata[0], val); return TRUE; } @@ -246,6 +351,13 @@ ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs, const char *key, GError **error) { + 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_remove_entries_from_order (kargs->order, entries); + if (!g_hash_table_remove (kargs->table, key)) { g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, @@ -254,10 +366,6 @@ ostree_kernel_args_delete_key_entry (OstreeKernelArgs *kargs, return FALSE; } - /* Then remove the key from order table */ - guint key_index; - g_assert (ot_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)); return TRUE; } @@ -294,16 +402,17 @@ ostree_kernel_args_delete (OstreeKernelArgs *kargs, const char *key = arg_owned; const char *val = split_keyeq (arg_owned); - GPtrArray *values = g_hash_table_lookup (kargs->table, key); - if (!values) + GPtrArray *entries = g_hash_table_lookup (kargs->table, key); + if (!entries) return glnx_throw (error, "No key '%s' found", key); - g_assert_cmpuint (values->len, >, 0); + g_assert_cmpuint (entries->len, >, 0); /* special-case: we allow deleting by key only if there's only one val */ - if (values->len == 1) + if (entries->len == 1) { /* but if a specific val was passed, check that it's the same */ - if (val && !strcmp0_equal (val, values->pdata[0])) + 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); } @@ -311,7 +420,7 @@ ostree_kernel_args_delete (OstreeKernelArgs *kargs, /* 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 (values, val, strcmp0_equal, &i)) + 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 @@ -320,7 +429,8 @@ ostree_kernel_args_delete (OstreeKernelArgs *kargs, return glnx_throw (error, "No karg '%s' found", arg); } - g_ptr_array_remove_index (values, i); + g_assert (g_ptr_array_remove (kargs->order, entries->pdata[i])); + g_assert (g_ptr_array_remove_index (entries, i)); return TRUE; } @@ -340,22 +450,38 @@ ostree_kernel_args_replace_take (OstreeKernelArgs *kargs, char *arg) { gboolean existed; - GPtrArray *values = g_ptr_array_new_with_free_func (g_free); + GPtrArray *entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table); const char *value = split_keyeq (arg); gpointer old_key; - g_ptr_array_add (values, g_strdup (value)); - existed = g_hash_table_lookup_extended (kargs->table, arg, &old_key, NULL); + OstreeKernelArgsEntry *entry = g_new0 (OstreeKernelArgsEntry, 1); + _ostree_kernel_args_entry_set_value (entry, g_strdup (value)); + g_ptr_array_add (entries, entry); + + gpointer old_entries_ptr; + existed = g_hash_table_lookup_extended (kargs->table, arg, &old_key, &old_entries_ptr); + GPtrArray *old_entries = old_entries_ptr; if (existed) { - g_hash_table_replace (kargs->table, old_key, values); - g_free (arg); + g_assert (old_entries); + g_assert_cmpuint (old_entries->len, >, 0); + + guint old_order_index = 0; + g_assert (ot_ptr_array_find_with_equal_func (kargs->order, old_key, kernel_args_entry_key_equal, &old_order_index)); + kernel_args_remove_entries_from_order (kargs->order, old_entries); + + g_assert_cmpstr (old_key, ==, arg); + _ostree_kernel_args_entry_set_key (entry, old_key); + g_ptr_array_insert (kargs->order, old_order_index, entry); + // `arg` is freed by the `g_hash_table_insert` call. + g_hash_table_insert (kargs->table, arg, entries); } else { - g_ptr_array_add (kargs->order, arg); - g_hash_table_replace (kargs->table, arg, values); + _ostree_kernel_args_entry_set_key (entry, arg); + g_hash_table_replace (kargs->table, arg, entries); + g_ptr_array_add (kargs->order, entry); } } @@ -393,28 +519,26 @@ ostree_kernel_args_append (OstreeKernelArgs *kargs, const char *arg) { gboolean existed = TRUE; - GPtrArray *values; + GPtrArray *entries = NULL; char *duped = g_strdup (arg); const char *val = split_keyeq (duped); - values = g_hash_table_lookup (kargs->table, duped); - if (!values) + entries = g_hash_table_lookup (kargs->table, duped); + if (!entries) { - values = g_ptr_array_new_with_free_func (g_free); + entries = g_ptr_array_new_with_free_func (kernel_args_entry_free_from_table); existed = FALSE; } - g_ptr_array_add (values, g_strdup (val)); + 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)); + + g_ptr_array_add (entries, entry); + g_ptr_array_add (kargs->order, entry); if (!existed) - { - g_hash_table_replace (kargs->table, duped, values); - g_ptr_array_add (kargs->order, duped); - } - else - { - g_free (duped); - } + g_hash_table_replace (kargs->table, duped, entries); } /** @@ -598,20 +722,13 @@ ostree_kernel_args_to_strv (OstreeKernelArgs *kargs) for (i = 0; i < kargs->order->len; i++) { - const char *key = kargs->order->pdata[i]; - GPtrArray *values = g_hash_table_lookup (kargs->table, key); - guint j; + const char *key = _ostree_kernel_args_get_key_index (kargs, i); + const char *value = _ostree_kernel_args_get_value_index (kargs, i); - g_assert (values != NULL); - - for (j = 0; j < values->len; j++) - { - const char *value = values->pdata[j]; - if (value == NULL) - g_ptr_array_add (strv, g_strconcat (key, NULL)); - else - g_ptr_array_add (strv, g_strconcat (key, "=", value, NULL)); - } + if (value == NULL) + g_ptr_array_add (strv, g_strconcat (key, NULL)); + else + g_ptr_array_add (strv, g_strconcat (key, "=", value, NULL)); } g_ptr_array_add (strv, NULL); @@ -644,27 +761,19 @@ ostree_kernel_args_to_string (OstreeKernelArgs *kargs) for (i = 0; i < kargs->order->len; i++) { - const char *key = kargs->order->pdata[i]; - GPtrArray *values = g_hash_table_lookup (kargs->table, key); - guint j; + const char *key = _ostree_kernel_args_get_key_index (kargs, i); + const char *value = _ostree_kernel_args_get_value_index (kargs, i); - g_assert (values != NULL); + if (first) + first = FALSE; + else + g_string_append_c (buf, ' '); - for (j = 0; j < values->len; j++) + g_string_append (buf, key); + if (value != NULL) { - const char *value = values->pdata[j]; - - if (first) - first = FALSE; - else - g_string_append_c (buf, ' '); - - g_string_append (buf, key); - if (value != NULL) - { - g_string_append_c (buf, '='); - g_string_append (buf, value); - } + g_string_append_c (buf, '='); + g_string_append (buf, value); } } @@ -688,11 +797,12 @@ ostree_kernel_args_to_string (OstreeKernelArgs *kargs) const char * ostree_kernel_args_get_last_value (OstreeKernelArgs *kargs, const char *key) { - GPtrArray *values = g_hash_table_lookup (kargs->table, key); + const GPtrArray *entries = g_hash_table_lookup (kargs->table, key); - if (!values) + if (!entries) return NULL; - g_assert (values->len > 0); - return (char*)values->pdata[values->len-1]; + g_assert (entries->len > 0); + const OstreeKernelArgsEntry *e = entries->pdata[entries->len-1]; + return _ostree_kernel_args_entry_get_value (e); } diff --git a/src/libostree/ostree-kernel-args.h b/src/libostree/ostree-kernel-args.h index 3975ae5c..5c8be0c0 100644 --- a/src/libostree/ostree-kernel-args.h +++ b/src/libostree/ostree-kernel-args.h @@ -27,11 +27,40 @@ G_BEGIN_DECLS typedef struct _OstreeKernelArgs OstreeKernelArgs; +typedef struct _OstreeKernelArgsEntry OstreeKernelArgsEntry; GHashTable *_ostree_kernel_arg_get_kargs_table (OstreeKernelArgs *kargs); GPtrArray *_ostree_kernel_arg_get_key_array (OstreeKernelArgs *kargs); +char * +_ostree_kernel_args_entry_get_key (const OstreeKernelArgsEntry *e); + +char * +_ostree_kernel_args_entry_get_value (const OstreeKernelArgsEntry *e); + +void +_ostree_kernel_args_entry_set_key (OstreeKernelArgsEntry *e, + char *key); + +void +_ostree_kernel_args_entry_set_value (OstreeKernelArgsEntry *e, + char *value); + +char * +_ostree_kernel_args_get_key_index (const OstreeKernelArgs *kargs, + int i); + +char * +_ostree_kernel_args_get_value_index (const OstreeKernelArgs *kargs, + int i); + +OstreeKernelArgsEntry * +_ostree_kernel_args_entry_new (void); + +void +_ostree_kernel_args_entry_value_free (OstreeKernelArgsEntry *e); + _OSTREE_PUBLIC void ostree_kernel_args_free (OstreeKernelArgs *kargs); diff --git a/tests/test-admin-deploy-karg.sh b/tests/test-admin-deploy-karg.sh index aade011c..ccf66b0e 100755 --- a/tests/test-admin-deploy-karg.sh +++ b/tests/test-admin-deploy-karg.sh @@ -66,4 +66,8 @@ assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'option assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*TESTARG=TESTVALUE' assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*APPENDARG=VALAPPEND .*APPENDARG=2NDAPPEND' +# Check correct ordering of different-valued args of the same key. +${CMD_PREFIX} ostree admin deploy --os=testos --karg-append=FOO=TESTORDERED --karg-append=APPENDARG=3RDAPPEND testos:testos/buildmaster/x86_64-runtime +assert_file_has_content sysroot/boot/loader/entries/ostree-2-testos.conf 'options.*APPENDARG=VALAPPEND .*APPENDARG=2NDAPPEND .*FOO=TESTORDERED .*APPENDARG=3RDAPPEND' + echo "ok deploy --karg-append" diff --git a/tests/test-kargs.c b/tests/test-kargs.c index 8d34f73c..d8370555 100644 --- a/tests/test-kargs.c +++ b/tests/test-kargs.c @@ -32,6 +32,22 @@ check_string_existance (OstreeKernelArgs *karg, return g_strv_contains ((const char* const*) string_list, string_to_find); } +static gboolean +kernel_args_entry_value_equal (gconstpointer data, + gconstpointer value) +{ + const OstreeKernelArgsEntry *e = data; + return g_strcmp0 (_ostree_kernel_args_entry_get_value (e), value) == 0; +} + +static gboolean +kernel_args_entry_key_equal (gconstpointer data, + gconstpointer key) +{ + const OstreeKernelArgsEntry *e = data; + return g_strcmp0 (_ostree_kernel_args_entry_get_key (e), key) == 0; +} + static void test_kargs_delete (void) { @@ -82,7 +98,7 @@ test_kargs_delete (void) g_assert (ret); /* verify the value array is properly updated */ GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (karg); - g_assert (!ot_ptr_array_find_with_equal_func (kargs_array, "single_key", g_str_equal, NULL)); + g_assert (!ot_ptr_array_find_with_equal_func (kargs_array, "single_key", kernel_args_entry_value_equal, NULL)); g_assert (!check_string_existance (karg, "single_key")); /* Delete specific key/value pair */ @@ -177,13 +193,6 @@ test_kargs_replace (void) 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 * and ostree_kernel_args_to_string is correct. After that * we will use these two functions(append and tostring) in other tests: delete and replace @@ -208,22 +217,22 @@ test_kargs_append (void) { if (g_str_equal (key, "test")) { - g_assert (ot_ptr_array_find_with_equal_func (value_array, "valid", strcmp0_equal, NULL)); - g_assert (ot_ptr_array_find_with_equal_func (value_array, "secondvalid", strcmp0_equal, NULL)); - g_assert (ot_ptr_array_find_with_equal_func (value_array, "", strcmp0_equal, NULL)); - g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, strcmp0_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, "valid", kernel_args_entry_value_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, "secondvalid", kernel_args_entry_value_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, "", kernel_args_entry_value_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, kernel_args_entry_value_equal, NULL)); } else { g_assert_cmpstr (key, ==, "second_test"); - g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, strcmp0_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (value_array, NULL, kernel_args_entry_value_equal, NULL)); } } /* verify the value array is properly updated */ GPtrArray *kargs_array = _ostree_kernel_arg_get_key_array (append_arg); - g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "test", g_str_equal, NULL)); - g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "second_test", g_str_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "test", kernel_args_entry_key_equal, NULL)); + g_assert (ot_ptr_array_find_with_equal_func (kargs_array, "second_test", kernel_args_entry_key_equal, NULL)); /* Up till this point, we verified that the above was all correct, we then * check ostree_kernel_args_to_string has the right result