diff mbox series

[RFC,v4,03/11] linux-user: Implement envlist_appendenv and add tests for envlist

Message ID 20230808141739.3110740-4-fufuyqqqqqq@gmail.com
State New
Headers show
Series Native Library Calls | expand

Commit Message

Yeqi Fu Aug. 8, 2023, 2:17 p.m. UTC 
Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
---
 include/qemu/envlist.h    | 13 ++++++
 tests/unit/meson.build    |  1 +
 tests/unit/test-envlist.c | 94 +++++++++++++++++++++++++++++++++++++++
 util/envlist.c            | 71 ++++++++++++++++++++++++-----
 4 files changed, 169 insertions(+), 10 deletions(-)
 create mode 100644 tests/unit/test-envlist.c

Comments

Alex Bennée Aug. 9, 2023, 3:27 p.m. UTC | #1 
Yeqi Fu <fufuyqqqqqq@gmail.com> writes:

> Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
> ---
>  include/qemu/envlist.h    | 13 ++++++
>  tests/unit/meson.build    |  1 +
>  tests/unit/test-envlist.c | 94 +++++++++++++++++++++++++++++++++++++++
>  util/envlist.c            | 71 ++++++++++++++++++++++++-----
>  4 files changed, 169 insertions(+), 10 deletions(-)
>  create mode 100644 tests/unit/test-envlist.c

Thanks for adding the unit test.
>  
> +/*
> + * Appends environment value to envlist. If the environment
> + * variable already exists, the new value is appended to the
> + * existing one.
> + *
> + * Returns 0 in success, errno otherwise.
> + */
> +int
> +envlist_appendenv(envlist_t *envlist, const char *env, const char *separator)
> +{
> +    struct envlist_entry *entry = NULL;
> +    const char *eq_sign;
> +    size_t envname_len;
> +
> +    if ((envlist == NULL) || (env == NULL) || (separator == NULL)) {
> +        return -EINVAL;
> +    }
> +
> +    /* find out first equals sign in given env */
> +    eq_sign = strchr(env, '=');
> +    if (eq_sign == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    if (strchr(eq_sign + 1, '=') != NULL) {
> +        return -EINVAL;
> +    }
> +
> +    envname_len = eq_sign - env + 1;
> +
> +    /*
> +     * If there already exists variable with given name,
> +     * we append the new value to the existing one.
> +     */
> +    for (entry = envlist->el_entries.lh_first; entry != NULL;
> +        entry = entry->ev_link.le_next) {
> +        if (strncmp(entry->ev_var, env, envname_len) == 0) {
> +            break;
> +        }
> +    }
> +
> +    if (entry != NULL) {
> +        char *new_env_value = NULL;
> +        size_t new_env_len = strlen(entry->ev_var) + strlen(eq_sign)
> +            + strlen(separator) + 1;
> +        new_env_value = g_malloc(new_env_len);
> +        strcpy(new_env_value, entry->ev_var);
> +        strcat(new_env_value, separator);
> +        strcat(new_env_value, eq_sign + 1);
> +        g_free((char *)entry->ev_var);
> +        entry->ev_var = new_env_value;

You could probably avoid messing about with strlens if you used the
handy glib functions:

    if (entry != NULL) {
        GString *new_val = g_string_new(entry->ev_var);
        g_string_append(new_val, separator);
        g_string_append(new_val, eq_sign + 1);
        g_free(entry->ev_var);
        entry->ev_var = g_string_free(new_val, false);
    } else {

Also the fact you needed to cast entry->ev_var to g_free() should have
pointed out that now we can change env entries we need to drop the const
from the char *ev_var; definition in envlist_entry.

Otherwise with those fixes:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Richard Henderson Aug. 9, 2023, 3:44 p.m. UTC | #2 
On 8/8/23 07:17, Yeqi Fu wrote:
> Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
> ---
>   include/qemu/envlist.h    | 13 ++++++
>   tests/unit/meson.build    |  1 +
>   tests/unit/test-envlist.c | 94 +++++++++++++++++++++++++++++++++++++++
>   util/envlist.c            | 71 ++++++++++++++++++++++++-----
>   4 files changed, 169 insertions(+), 10 deletions(-)
>   create mode 100644 tests/unit/test-envlist.c
> 
> diff --git a/include/qemu/envlist.h b/include/qemu/envlist.h
> index 6006dfae44..9eb1459e79 100644
> --- a/include/qemu/envlist.h
> +++ b/include/qemu/envlist.h
> @@ -1,12 +1,25 @@
>   #ifndef ENVLIST_H
>   #define ENVLIST_H
>   
> +#include "qemu/queue.h"
> +
> +struct envlist_entry {
> +    const char *ev_var;            /* actual env value */
> +    QLIST_ENTRY(envlist_entry) ev_link;
> +};
> +
> +struct envlist {
> +    QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
> +    size_t el_count;                        /* number of entries */
> +};
> +

Why are you exposing the structures?

> +static void envlist_parse_set_unset_test(void)
> +{
> +    envlist_t *testenvlist;
> +    const char *env = "TEST1=123,TEST2=456";
> +
> +    testenvlist = envlist_create();
> +    g_assert(envlist_parse_set(testenvlist, env) == 0);
> +    g_assert(testenvlist->el_count == 2);
> +    g_assert(envlist_parse_unset(testenvlist, "TEST1,TEST2") == 0);
> +    g_assert(testenvlist->el_count == 0);

If it's just for the count, then add an envlist_length() function.


r~
Richard Henderson Aug. 9, 2023, 4:06 p.m. UTC | #3 
On 8/8/23 07:17, Yeqi Fu wrote:
> +        char *new_env_value = NULL;
> +        size_t new_env_len = strlen(entry->ev_var) + strlen(eq_sign)
> +            + strlen(separator) + 1;
> +        new_env_value = g_malloc(new_env_len);
> +        strcpy(new_env_value, entry->ev_var);
> +        strcat(new_env_value, separator);
> +        strcat(new_env_value, eq_sign + 1);

This is

	new_env_value = g_strconcat(entry->ev_var, separator, eq_sign + 1, NULL);


r~
diff mbox series

Patch

diff --git a/include/qemu/envlist.h b/include/qemu/envlist.h
index 6006dfae44..9eb1459e79 100644
--- a/include/qemu/envlist.h
+++ b/include/qemu/envlist.h
@@ -1,12 +1,25 @@ 
 #ifndef ENVLIST_H
 #define ENVLIST_H
 
+#include "qemu/queue.h"
+
+struct envlist_entry {
+    const char *ev_var;            /* actual env value */
+    QLIST_ENTRY(envlist_entry) ev_link;
+};
+
+struct envlist {
+    QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
+    size_t el_count;                        /* number of entries */
+};
+
 typedef struct envlist envlist_t;
 
 envlist_t *envlist_create(void);
 void envlist_free(envlist_t *);
 int envlist_setenv(envlist_t *, const char *);
 int envlist_unsetenv(envlist_t *, const char *);
+int envlist_appendenv(envlist_t *, const char *, const char *);
 int envlist_parse_set(envlist_t *, const char *);
 int envlist_parse_unset(envlist_t *, const char *);
 char **envlist_to_environ(const envlist_t *, size_t *);
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 93977cc32d..9b25b45271 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -50,6 +50,7 @@  tests = {
   'test-qapi-util': [],
   'test-interval-tree': [],
   'test-xs-node': [qom],
+  'test-envlist': [],
 }
 
 if have_system or have_tools
diff --git a/tests/unit/test-envlist.c b/tests/unit/test-envlist.c
new file mode 100644
index 0000000000..d1e148f738
--- /dev/null
+++ b/tests/unit/test-envlist.c
@@ -0,0 +1,94 @@ 
+/*
+ * testenvlist unit-tests.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/envlist.h"
+
+static void envlist_create_free_test(void)
+{
+    envlist_t *testenvlist;
+
+    testenvlist = envlist_create();
+    g_assert(testenvlist != NULL);
+    g_assert(testenvlist->el_count == 0);
+
+    envlist_free(testenvlist);
+}
+
+static void envlist_set_unset_test(void)
+{
+    envlist_t *testenvlist;
+    const char *env = "TEST=123";
+    struct envlist_entry *entry;
+
+    testenvlist = envlist_create();
+    g_assert(envlist_setenv(testenvlist, env) == 0);
+    g_assert(testenvlist->el_count == 1);
+    entry = testenvlist->el_entries.lh_first;
+    g_assert_cmpstr(entry->ev_var, ==, "TEST=123");
+    g_assert(envlist_unsetenv(testenvlist, "TEST") == 0);
+    g_assert(testenvlist->el_count == 0);
+
+    envlist_free(testenvlist);
+}
+
+static void envlist_parse_set_unset_test(void)
+{
+    envlist_t *testenvlist;
+    const char *env = "TEST1=123,TEST2=456";
+
+    testenvlist = envlist_create();
+    g_assert(envlist_parse_set(testenvlist, env) == 0);
+    g_assert(testenvlist->el_count == 2);
+    g_assert(envlist_parse_unset(testenvlist, "TEST1,TEST2") == 0);
+    g_assert(testenvlist->el_count == 0);
+
+    envlist_free(testenvlist);
+}
+
+static void envlist_appendenv_test(void)
+{
+    envlist_t *testenvlist;
+    const char *env = "TEST=123";
+    struct envlist_entry *entry;
+
+    testenvlist = envlist_create();
+    g_assert(envlist_setenv(testenvlist, env) == 0);
+    g_assert(envlist_appendenv(testenvlist, "TEST=456", ";") == 0);
+    entry = testenvlist->el_entries.lh_first;
+    g_assert_cmpstr(entry->ev_var, ==, "TEST=123;456");
+
+    envlist_free(testenvlist);
+}
+
+static void envlist_to_environ_test(void)
+{
+    envlist_t *testenvlist;
+    const char *env = "TEST1=123,TEST2=456";
+    size_t count;
+    char **environ;
+
+    testenvlist = envlist_create();
+    g_assert(envlist_parse_set(testenvlist, env) == 0);
+    g_assert(testenvlist->el_count == 2);
+    environ = envlist_to_environ(testenvlist, &count);
+    g_assert(count == 2);
+    g_assert_cmpstr(environ[1], ==, "TEST1=123");
+    g_assert_cmpstr(environ[0], ==, "TEST2=456");
+
+    envlist_free(testenvlist);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/envlist/create_free", envlist_create_free_test);
+    g_test_add_func("/envlist/set_unset", envlist_set_unset_test);
+    g_test_add_func("/envlist/parse_set_unset", envlist_parse_set_unset_test);
+    g_test_add_func("/envlist/appendenv", envlist_appendenv_test);
+    g_test_add_func("/envlist/to_environ", envlist_to_environ_test);
+
+    return g_test_run();
+}
diff --git a/util/envlist.c b/util/envlist.c
index db937c0427..2570f0e30c 100644
--- a/util/envlist.c
+++ b/util/envlist.c
@@ -2,16 +2,6 @@ 
 #include "qemu/queue.h"
 #include "qemu/envlist.h"
 
-struct envlist_entry {
-    const char *ev_var;            /* actual env value */
-    QLIST_ENTRY(envlist_entry) ev_link;
-};
-
-struct envlist {
-    QLIST_HEAD(, envlist_entry) el_entries; /* actual entries */
-    size_t el_count;                        /* number of entries */
-};
-
 static int envlist_parse(envlist_t *envlist,
     const char *env, int (*)(envlist_t *, const char *));
 
@@ -201,6 +191,67 @@  envlist_unsetenv(envlist_t *envlist, const char *env)
     return (0);
 }
 
+/*
+ * Appends environment value to envlist. If the environment
+ * variable already exists, the new value is appended to the
+ * existing one.
+ *
+ * Returns 0 in success, errno otherwise.
+ */
+int
+envlist_appendenv(envlist_t *envlist, const char *env, const char *separator)
+{
+    struct envlist_entry *entry = NULL;
+    const char *eq_sign;
+    size_t envname_len;
+
+    if ((envlist == NULL) || (env == NULL) || (separator == NULL)) {
+        return -EINVAL;
+    }
+
+    /* find out first equals sign in given env */
+    eq_sign = strchr(env, '=');
+    if (eq_sign == NULL) {
+        return -EINVAL;
+    }
+
+    if (strchr(eq_sign + 1, '=') != NULL) {
+        return -EINVAL;
+    }
+
+    envname_len = eq_sign - env + 1;
+
+    /*
+     * If there already exists variable with given name,
+     * we append the new value to the existing one.
+     */
+    for (entry = envlist->el_entries.lh_first; entry != NULL;
+        entry = entry->ev_link.le_next) {
+        if (strncmp(entry->ev_var, env, envname_len) == 0) {
+            break;
+        }
+    }
+
+    if (entry != NULL) {
+        char *new_env_value = NULL;
+        size_t new_env_len = strlen(entry->ev_var) + strlen(eq_sign)
+            + strlen(separator) + 1;
+        new_env_value = g_malloc(new_env_len);
+        strcpy(new_env_value, entry->ev_var);
+        strcat(new_env_value, separator);
+        strcat(new_env_value, eq_sign + 1);
+        g_free((char *)entry->ev_var);
+        entry->ev_var = new_env_value;
+    } else {
+        envlist->el_count++;
+        entry = g_malloc(sizeof(*entry));
+        entry->ev_var = g_strdup(env);
+        QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
+    }
+
+    return 0;
+}
+
 /*
  * Returns given envlist as array of strings (in same form that
  * global variable environ is).  Caller must free returned memory