[PATCH V2 0/4] string list functions

[PATCH V2 0/4] string list functions

[Steve Sistare]

Add some handy string list functions, for general use now, and for
eventual use in the cpr/live update patches.

Steve Sistare (4):
  qapi: strList_from_string
  qapi: QAPI_LIST_LENGTH
  qapi: strv_from_strList
  qapi: strList unit tests

 include/monitor/hmp.h     |  1 -
 include/qapi/util.h       | 28 ++++++++++++++++
 monitor/hmp-cmds.c        | 19 -----------
 net/net-hmp-cmds.c        |  2 +-
 qapi/qapi-util.c          | 37 ++++++++++++++++++++++
 stats/stats-hmp-cmds.c    |  2 +-
 tests/unit/meson.build    |  1 +
 tests/unit/test-strlist.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 149 insertions(+), 22 deletions(-)
 create mode 100644 tests/unit/test-strlist.c

-- 
1.8.3.1

[PATCH V2 1/4] qapi: strList_from_string

[Steve Sistare]

Generalize hmp_split_at_comma() to take any delimiter character, rename
as strList_from_string(), and move it to qapi/util.c.

No functional change.

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 include/monitor/hmp.h  |  1 -
 include/qapi/util.h    |  9 +++++++++
 monitor/hmp-cmds.c     | 19 -------------------
 net/net-hmp-cmds.c     |  2 +-
 qapi/qapi-util.c       | 23 +++++++++++++++++++++++
 stats/stats-hmp-cmds.c |  2 +-
 6 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 2220f14..e80848f 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -19,7 +19,6 @@
 
 bool hmp_handle_error(Monitor *mon, Error *err);
 void hmp_help_cmd(Monitor *mon, const char *name);
-strList *hmp_split_at_comma(const char *str);
 
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
diff --git a/include/qapi/util.h b/include/qapi/util.h
index 81a2b13..7d88b09 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -22,6 +22,8 @@ typedef struct QEnumLookup {
     const int size;
 } QEnumLookup;
 
+struct strList;
+
 const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
 int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
                     int def, Error **errp);
@@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
 int parse_qapi_name(const char *name, bool complete);
 
 /*
+ * Produce a strList from the character delimited string @in.
+ * All strings are g_strdup'd.
+ * A NULL or empty input string returns NULL.
+ */
+struct strList *strList_from_string(const char *in, char delim);
+
+/*
  * For any GenericList @list, insert @element at the front.
  *
  * Note that this macro evaluates @element exactly once, so it is safe
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 34bd8c6..9665e6e 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
     return false;
 }
 
-/*
- * Split @str at comma.
- * A null @str defaults to "".
- */
-strList *hmp_split_at_comma(const char *str)
-{
-    char **split = g_strsplit(str ?: "", ",", -1);
-    strList *res = NULL;
-    strList **tail = &res;
-    int i;
-
-    for (i = 0; split[i]; i++) {
-        QAPI_LIST_APPEND(tail, split[i]);
-    }
-
-    g_free(split);
-    return res;
-}
-
 void hmp_info_name(Monitor *mon, const QDict *qdict)
 {
     NameInfo *info;
diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c
index 41d326b..30422d9 100644
--- a/net/net-hmp-cmds.c
+++ b/net/net-hmp-cmds.c
@@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
                                             migrate_announce_params());
 
     qapi_free_strList(params->interfaces);
-    params->interfaces = hmp_split_at_comma(interfaces_str);
+    params->interfaces = strList_from_string(interfaces_str, ',');
     params->has_interfaces = params->interfaces != NULL;
     params->id = g_strdup(id);
     qmp_announce_self(params, NULL);
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 63596e1..b61c73c 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -15,6 +15,7 @@
 #include "qapi/error.h"
 #include "qemu/ctype.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qapi-builtin-types.h"
 
 CompatPolicy compat_policy;
 
@@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete)
     }
     return p - str;
 }
+
+strList *strList_from_string(const char *in, char delim)
+{
+    strList *res = NULL;
+    strList **tail = &res;
+
+    while (in && in[0]) {
+        char *next = strchr(in, delim);
+        char *value;
+
+        if (next) {
+            value = g_strndup(in, next - in);
+            in = next + 1; /* skip the delim */
+        } else {
+            value = g_strdup(in);
+            in = NULL;
+        }
+        QAPI_LIST_APPEND(tail, value);
+    }
+
+    return res;
+}
diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c
index 531e35d..4a2adf8 100644
--- a/stats/stats-hmp-cmds.c
+++ b/stats/stats-hmp-cmds.c
@@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
             request->provider = provider_idx;
             if (names && !g_str_equal(names, "*")) {
                 request->has_names = true;
-                request->names = hmp_split_at_comma(names);
+                request->names = strList_from_string(names, ',');
             }
             QAPI_LIST_PREPEND(request_list, request);
         }
-- 
1.8.3.1

[PATCH V2 2/4] qapi: QAPI_LIST_LENGTH

[Steve Sistare]

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/util.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 7d88b09..75dddca 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -65,4 +65,17 @@ struct strList *strList_from_string(const char *in, char delim);
     (tail) = &(*(tail))->next; \
 } while (0)
 
+/*
+ * For any GenericList @list, return its length.
+ */
+#define QAPI_LIST_LENGTH(list) \
+    ({ \
+        int len = 0; \
+        typeof(list) elem; \
+        for (elem = list; elem != NULL; elem = elem->next) { \
+            len++; \
+        } \
+        len; \
+    })
+
 #endif
-- 
1.8.3.1

[PATCH V2 3/4] qapi: strv_from_strList

[Steve Sistare]

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qapi/util.h |  6 ++++++
 qapi/qapi-util.c    | 14 ++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/qapi/util.h b/include/qapi/util.h
index 75dddca..51ff64e 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -33,6 +33,12 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
 int parse_qapi_name(const char *name, bool complete);
 
 /*
+ * Produce and return a NULL-terminated array of strings from @args.
+ * All strings are g_strdup'd.
+ */
+GStrv strv_from_strList(const struct strList *args);
+
+/*
  * Produce a strList from the character delimited string @in.
  * All strings are g_strdup'd.
  * A NULL or empty input string returns NULL.
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index b61c73c..fe6bda2 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -154,6 +154,20 @@ int parse_qapi_name(const char *str, bool complete)
     return p - str;
 }
 
+GStrv strv_from_strList(const strList *args)
+{
+    const strList *arg;
+    int i = 0;
+    GStrv argv = g_new(char *, QAPI_LIST_LENGTH(args) + 1);
+
+    for (arg = args; arg != NULL; arg = arg->next) {
+        argv[i++] = g_strdup(arg->value);
+    }
+    argv[i] = NULL;
+
+    return argv;
+}
+
 strList *strList_from_string(const char *in, char delim)
 {
     strList *res = NULL;
-- 
1.8.3.1

[PATCH V2 4/4] qapi: strList unit tests

[Steve Sistare]

Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/unit/meson.build    |  1 +
 tests/unit/test-strlist.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 tests/unit/test-strlist.c

diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index ffa444f..43c3bee 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -17,6 +17,7 @@ tests = {
   'test-forward-visitor': [testqapi],
   'test-string-input-visitor': [testqapi],
   'test-string-output-visitor': [testqapi],
+  'test-strlist': [testqapi],
   'test-opts-visitor': [testqapi],
   'test-visitor-serialization': [testqapi],
   'test-bitmap': [],
diff --git a/tests/unit/test-strlist.c b/tests/unit/test-strlist.c
new file mode 100644
index 0000000..ef740dc
--- /dev/null
+++ b/tests/unit/test-strlist.c
@@ -0,0 +1,81 @@
+/*
+ * Copyright (c) 2022 Oracle and/or its affiliates.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/util.h"
+#include "qapi/qapi-builtin-types.h"
+
+static strList *make_list(int length)
+{
+    strList *head = 0, *list, **prev = &head;
+
+    while (length--) {
+        list = *prev = g_new0(strList, 1);
+        list->value = g_strdup("aaa");
+        prev = &list->next;
+    }
+    return head;
+}
+
+static void test_length(void)
+{
+    strList *list;
+    int i;
+
+    for (i = 0; i < 5; i++) {
+        list = make_list(i);
+        g_assert_cmpint(i, ==, QAPI_LIST_LENGTH(list));
+        qapi_free_strList(list);
+    }
+}
+
+struct {
+    const char *string;
+    char delim;
+    const char *args[5];
+} list_data[] = {
+    { 0, ',', { 0 } },
+    { "", ',', { 0 } },
+    { "a", ',', { "a", 0 } },
+    { "a,b", ',', { "a", "b", 0 } },
+    { "a,b,c", ',', { "a", "b", "c", 0 } },
+    { "first last", ' ', { "first", "last", 0 } },
+    { "a:", ':', { "a", 0 } },
+    { "a::b", ':', { "a", "", "b", 0 } },
+    { ":", ':', { "", 0 } },
+    { ":a", ':', { "", "a", 0 } },
+    { "::a", ':', { "", "", "a", 0 } },
+};
+
+static void test_strv(void)
+{
+    int i, j;
+    const char **expect;
+    strList *list;
+    GStrv args;
+
+    for (i = 0; i < ARRAY_SIZE(list_data); i++) {
+        expect = list_data[i].args;
+        list = strList_from_string(list_data[i].string, list_data[i].delim);
+        args = strv_from_strList(list);
+        qapi_free_strList(list);
+        for (j = 0; expect[j] && args[j]; j++) {
+            g_assert_cmpstr(expect[j], ==, args[j]);
+        }
+        g_assert_null(expect[j]);
+        g_assert_null(args[j]);
+        g_strfreev(args);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+    g_test_add_func("/test-string/length", test_length);
+    g_test_add_func("/test-string/strv", test_strv);
+    return g_test_run();
+}
-- 
1.8.3.1

Re: [PATCH V2 1/4] qapi: strList_from_string

[Marc-André Lureau]

Hi

On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>
> Generalize hmp_split_at_comma() to take any delimiter character, rename
> as strList_from_string(), and move it to qapi/util.c.
>
> No functional change.

The g_strsplit() version was a bit simpler, but if you want to
optimize it a bit for 1 char delimiter only, ok.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  include/monitor/hmp.h  |  1 -
>  include/qapi/util.h    |  9 +++++++++
>  monitor/hmp-cmds.c     | 19 -------------------
>  net/net-hmp-cmds.c     |  2 +-
>  qapi/qapi-util.c       | 23 +++++++++++++++++++++++
>  stats/stats-hmp-cmds.c |  2 +-
>  6 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index 2220f14..e80848f 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -19,7 +19,6 @@
>
>  bool hmp_handle_error(Monitor *mon, Error *err);
>  void hmp_help_cmd(Monitor *mon, const char *name);
> -strList *hmp_split_at_comma(const char *str);
>
>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>  void hmp_info_version(Monitor *mon, const QDict *qdict);
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 81a2b13..7d88b09 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -22,6 +22,8 @@ typedef struct QEnumLookup {
>      const int size;
>  } QEnumLookup;
>
> +struct strList;
> +
>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
>  int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>                      int def, Error **errp);
> @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
>  int parse_qapi_name(const char *name, bool complete);
>
>  /*
> + * Produce a strList from the character delimited string @in.
> + * All strings are g_strdup'd.
> + * A NULL or empty input string returns NULL.
> + */
> +struct strList *strList_from_string(const char *in, char delim);
> +
> +/*
>   * For any GenericList @list, insert @element at the front.
>   *
>   * Note that this macro evaluates @element exactly once, so it is safe
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 34bd8c6..9665e6e 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
>      return false;
>  }
>
> -/*
> - * Split @str at comma.
> - * A null @str defaults to "".
> - */
> -strList *hmp_split_at_comma(const char *str)
> -{
> -    char **split = g_strsplit(str ?: "", ",", -1);
> -    strList *res = NULL;
> -    strList **tail = &res;
> -    int i;
> -
> -    for (i = 0; split[i]; i++) {
> -        QAPI_LIST_APPEND(tail, split[i]);
> -    }
> -
> -    g_free(split);
> -    return res;
> -}
> -
>  void hmp_info_name(Monitor *mon, const QDict *qdict)
>  {
>      NameInfo *info;
> diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c
> index 41d326b..30422d9 100644
> --- a/net/net-hmp-cmds.c
> +++ b/net/net-hmp-cmds.c
> @@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
>                                              migrate_announce_params());
>
>      qapi_free_strList(params->interfaces);
> -    params->interfaces = hmp_split_at_comma(interfaces_str);
> +    params->interfaces = strList_from_string(interfaces_str, ',');
>      params->has_interfaces = params->interfaces != NULL;
>      params->id = g_strdup(id);
>      qmp_announce_self(params, NULL);
> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
> index 63596e1..b61c73c 100644
> --- a/qapi/qapi-util.c
> +++ b/qapi/qapi-util.c
> @@ -15,6 +15,7 @@
>  #include "qapi/error.h"
>  #include "qemu/ctype.h"
>  #include "qapi/qmp/qerror.h"
> +#include "qapi/qapi-builtin-types.h"
>
>  CompatPolicy compat_policy;
>
> @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete)
>      }
>      return p - str;
>  }
> +
> +strList *strList_from_string(const char *in, char delim)
> +{
> +    strList *res = NULL;
> +    strList **tail = &res;
> +
> +    while (in && in[0]) {
> +        char *next = strchr(in, delim);
> +        char *value;
> +
> +        if (next) {
> +            value = g_strndup(in, next - in);
> +            in = next + 1; /* skip the delim */
> +        } else {
> +            value = g_strdup(in);
> +            in = NULL;
> +        }
> +        QAPI_LIST_APPEND(tail, value);
> +    }
> +
> +    return res;
> +}
> diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c
> index 531e35d..4a2adf8 100644
> --- a/stats/stats-hmp-cmds.c
> +++ b/stats/stats-hmp-cmds.c
> @@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
>              request->provider = provider_idx;
>              if (names && !g_str_equal(names, "*")) {
>                  request->has_names = true;
> -                request->names = hmp_split_at_comma(names);
> +                request->names = strList_from_string(names, ',');
>              }
>              QAPI_LIST_PREPEND(request_list, request);
>          }
> --
> 1.8.3.1
>
>


-- 
Marc-André Lureau

Re: [PATCH V2 1/4] qapi: strList_from_string

[Steven Sistare]

On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>
>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>> as strList_from_string(), and move it to qapi/util.c.
>>
>> No functional change.
> 
> The g_strsplit() version was a bit simpler, but if you want to
> optimize it a bit for 1 char delimiter only, ok.
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Yes, and it saves a malloc+free for the array.  Small stuff, but I
thought it worth a few lines of code.  Thanks for the speedy review!

- Steve

>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  include/monitor/hmp.h  |  1 -
>>  include/qapi/util.h    |  9 +++++++++
>>  monitor/hmp-cmds.c     | 19 -------------------
>>  net/net-hmp-cmds.c     |  2 +-
>>  qapi/qapi-util.c       | 23 +++++++++++++++++++++++
>>  stats/stats-hmp-cmds.c |  2 +-
>>  6 files changed, 34 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index 2220f14..e80848f 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -19,7 +19,6 @@
>>
>>  bool hmp_handle_error(Monitor *mon, Error *err);
>>  void hmp_help_cmd(Monitor *mon, const char *name);
>> -strList *hmp_split_at_comma(const char *str);
>>
>>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>>  void hmp_info_version(Monitor *mon, const QDict *qdict);
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 81a2b13..7d88b09 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -22,6 +22,8 @@ typedef struct QEnumLookup {
>>      const int size;
>>  } QEnumLookup;
>>
>> +struct strList;
>> +
>>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
>>  int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>>                      int def, Error **errp);
>> @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
>>  int parse_qapi_name(const char *name, bool complete);
>>
>>  /*
>> + * Produce a strList from the character delimited string @in.
>> + * All strings are g_strdup'd.
>> + * A NULL or empty input string returns NULL.
>> + */
>> +struct strList *strList_from_string(const char *in, char delim);
>> +
>> +/*
>>   * For any GenericList @list, insert @element at the front.
>>   *
>>   * Note that this macro evaluates @element exactly once, so it is safe
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index 34bd8c6..9665e6e 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
>>      return false;
>>  }
>>
>> -/*
>> - * Split @str at comma.
>> - * A null @str defaults to "".
>> - */
>> -strList *hmp_split_at_comma(const char *str)
>> -{
>> -    char **split = g_strsplit(str ?: "", ",", -1);
>> -    strList *res = NULL;
>> -    strList **tail = &res;
>> -    int i;
>> -
>> -    for (i = 0; split[i]; i++) {
>> -        QAPI_LIST_APPEND(tail, split[i]);
>> -    }
>> -
>> -    g_free(split);
>> -    return res;
>> -}
>> -
>>  void hmp_info_name(Monitor *mon, const QDict *qdict)
>>  {
>>      NameInfo *info;
>> diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c
>> index 41d326b..30422d9 100644
>> --- a/net/net-hmp-cmds.c
>> +++ b/net/net-hmp-cmds.c
>> @@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
>>                                              migrate_announce_params());
>>
>>      qapi_free_strList(params->interfaces);
>> -    params->interfaces = hmp_split_at_comma(interfaces_str);
>> +    params->interfaces = strList_from_string(interfaces_str, ',');
>>      params->has_interfaces = params->interfaces != NULL;
>>      params->id = g_strdup(id);
>>      qmp_announce_self(params, NULL);
>> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>> index 63596e1..b61c73c 100644
>> --- a/qapi/qapi-util.c
>> +++ b/qapi/qapi-util.c
>> @@ -15,6 +15,7 @@
>>  #include "qapi/error.h"
>>  #include "qemu/ctype.h"
>>  #include "qapi/qmp/qerror.h"
>> +#include "qapi/qapi-builtin-types.h"
>>
>>  CompatPolicy compat_policy;
>>
>> @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete)
>>      }
>>      return p - str;
>>  }
>> +
>> +strList *strList_from_string(const char *in, char delim)
>> +{
>> +    strList *res = NULL;
>> +    strList **tail = &res;
>> +
>> +    while (in && in[0]) {
>> +        char *next = strchr(in, delim);
>> +        char *value;
>> +
>> +        if (next) {
>> +            value = g_strndup(in, next - in);
>> +            in = next + 1; /* skip the delim */
>> +        } else {
>> +            value = g_strdup(in);
>> +            in = NULL;
>> +        }
>> +        QAPI_LIST_APPEND(tail, value);
>> +    }
>> +
>> +    return res;
>> +}
>> diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c
>> index 531e35d..4a2adf8 100644
>> --- a/stats/stats-hmp-cmds.c
>> +++ b/stats/stats-hmp-cmds.c
>> @@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
>>              request->provider = provider_idx;
>>              if (names && !g_str_equal(names, "*")) {
>>                  request->has_names = true;
>> -                request->names = hmp_split_at_comma(names);
>> +                request->names = strList_from_string(names, ',');
>>              }
>>              QAPI_LIST_PREPEND(request_list, request);
>>          }
>> --
>> 1.8.3.1
>>
>>
> 
> 

Re: [PATCH V2 1/4] qapi: strList_from_string

[Alex Bennée]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>> Hi
>> 
>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>
>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>> as strList_from_string(), and move it to qapi/util.c.
>>>
>>> No functional change.
>> 
>> The g_strsplit() version was a bit simpler, but if you want to
>> optimize it a bit for 1 char delimiter only, ok.
>> 
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Yes, and it saves a malloc+free for the array.  Small stuff, but I
> thought it worth a few lines of code.  Thanks for the speedy review!

But is the HMP path that performance critical? Otherwise I'd favour
consistent use of the glib APIs because its one less thing to get wrong.

>
> - Steve
>
>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>> ---
>>>  include/monitor/hmp.h  |  1 -
>>>  include/qapi/util.h    |  9 +++++++++
>>>  monitor/hmp-cmds.c     | 19 -------------------
>>>  net/net-hmp-cmds.c     |  2 +-
>>>  qapi/qapi-util.c       | 23 +++++++++++++++++++++++
>>>  stats/stats-hmp-cmds.c |  2 +-
>>>  6 files changed, 34 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>>> index 2220f14..e80848f 100644
>>> --- a/include/monitor/hmp.h
>>> +++ b/include/monitor/hmp.h
>>> @@ -19,7 +19,6 @@
>>>
>>>  bool hmp_handle_error(Monitor *mon, Error *err);
>>>  void hmp_help_cmd(Monitor *mon, const char *name);
>>> -strList *hmp_split_at_comma(const char *str);
>>>
>>>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>>>  void hmp_info_version(Monitor *mon, const QDict *qdict);
>>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>>> index 81a2b13..7d88b09 100644
>>> --- a/include/qapi/util.h
>>> +++ b/include/qapi/util.h
>>> @@ -22,6 +22,8 @@ typedef struct QEnumLookup {
>>>      const int size;
>>>  } QEnumLookup;
>>>
>>> +struct strList;
>>> +
>>>  const char *qapi_enum_lookup(const QEnumLookup *lookup, int val);
>>>  int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>>>                      int def, Error **errp);
>>> @@ -31,6 +33,13 @@ bool qapi_bool_parse(const char *name, const char *value, bool *obj,
>>>  int parse_qapi_name(const char *name, bool complete);
>>>
>>>  /*
>>> + * Produce a strList from the character delimited string @in.
>>> + * All strings are g_strdup'd.
>>> + * A NULL or empty input string returns NULL.
>>> + */
>>> +struct strList *strList_from_string(const char *in, char delim);
>>> +
>>> +/*
>>>   * For any GenericList @list, insert @element at the front.
>>>   *
>>>   * Note that this macro evaluates @element exactly once, so it is safe
>>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>>> index 34bd8c6..9665e6e 100644
>>> --- a/monitor/hmp-cmds.c
>>> +++ b/monitor/hmp-cmds.c
>>> @@ -39,25 +39,6 @@ bool hmp_handle_error(Monitor *mon, Error *err)
>>>      return false;
>>>  }
>>>
>>> -/*
>>> - * Split @str at comma.
>>> - * A null @str defaults to "".
>>> - */
>>> -strList *hmp_split_at_comma(const char *str)
>>> -{
>>> -    char **split = g_strsplit(str ?: "", ",", -1);
>>> -    strList *res = NULL;
>>> -    strList **tail = &res;
>>> -    int i;
>>> -
>>> -    for (i = 0; split[i]; i++) {
>>> -        QAPI_LIST_APPEND(tail, split[i]);
>>> -    }
>>> -
>>> -    g_free(split);
>>> -    return res;
>>> -}
>>> -
>>>  void hmp_info_name(Monitor *mon, const QDict *qdict)
>>>  {
>>>      NameInfo *info;
>>> diff --git a/net/net-hmp-cmds.c b/net/net-hmp-cmds.c
>>> index 41d326b..30422d9 100644
>>> --- a/net/net-hmp-cmds.c
>>> +++ b/net/net-hmp-cmds.c
>>> @@ -72,7 +72,7 @@ void hmp_announce_self(Monitor *mon, const QDict *qdict)
>>>                                              migrate_announce_params());
>>>
>>>      qapi_free_strList(params->interfaces);
>>> -    params->interfaces = hmp_split_at_comma(interfaces_str);
>>> +    params->interfaces = strList_from_string(interfaces_str, ',');
>>>      params->has_interfaces = params->interfaces != NULL;
>>>      params->id = g_strdup(id);
>>>      qmp_announce_self(params, NULL);
>>> diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>>> index 63596e1..b61c73c 100644
>>> --- a/qapi/qapi-util.c
>>> +++ b/qapi/qapi-util.c
>>> @@ -15,6 +15,7 @@
>>>  #include "qapi/error.h"
>>>  #include "qemu/ctype.h"
>>>  #include "qapi/qmp/qerror.h"
>>> +#include "qapi/qapi-builtin-types.h"
>>>
>>>  CompatPolicy compat_policy;
>>>
>>> @@ -152,3 +153,25 @@ int parse_qapi_name(const char *str, bool complete)
>>>      }
>>>      return p - str;
>>>  }
>>> +
>>> +strList *strList_from_string(const char *in, char delim)
>>> +{
>>> +    strList *res = NULL;
>>> +    strList **tail = &res;
>>> +
>>> +    while (in && in[0]) {
>>> +        char *next = strchr(in, delim);
>>> +        char *value;
>>> +
>>> +        if (next) {
>>> +            value = g_strndup(in, next - in);
>>> +            in = next + 1; /* skip the delim */
>>> +        } else {
>>> +            value = g_strdup(in);
>>> +            in = NULL;
>>> +        }
>>> +        QAPI_LIST_APPEND(tail, value);
>>> +    }
>>> +
>>> +    return res;
>>> +}
>>> diff --git a/stats/stats-hmp-cmds.c b/stats/stats-hmp-cmds.c
>>> index 531e35d..4a2adf8 100644
>>> --- a/stats/stats-hmp-cmds.c
>>> +++ b/stats/stats-hmp-cmds.c
>>> @@ -174,7 +174,7 @@ static StatsFilter *stats_filter(StatsTarget target, const char *names,
>>>              request->provider = provider_idx;
>>>              if (names && !g_str_equal(names, "*")) {
>>>                  request->has_names = true;
>>> -                request->names = hmp_split_at_comma(names);
>>> +                request->names = strList_from_string(names, ',');
>>>              }
>>>              QAPI_LIST_PREPEND(request_list, request);
>>>          }
>>> --
>>> 1.8.3.1
>>>
>>>
>> 
>> 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH V2 1/4] qapi: strList_from_string

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> Steven Sistare <steven.sistare@oracle.com> writes:
>
>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>>> Hi
>>> 
>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>
>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>>> as strList_from_string(), and move it to qapi/util.c.
>>>>
>>>> No functional change.
>>> 
>>> The g_strsplit() version was a bit simpler, but if you want to
>>> optimize it a bit for 1 char delimiter only, ok.
>>> 
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Yes, and it saves a malloc+free for the array.  Small stuff, but I
>> thought it worth a few lines of code.  Thanks for the speedy review!
>
> But is the HMP path that performance critical? Otherwise I'd favour
> consistent use of the glib APIs because its one less thing to get wrong.

The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
strlist_from_comma_list() as hmp_split_at_comma()", with a different
function name and place, and an additional parameter.

There is no explanation for the revert.

An intentional revert without even mentioning it would be uncourteous.
I don't think this is the case here.  I figure you wrote this patch
before you saw my commit, then rebased, keeping the old code.  A simple
rebase mistake, easy enough to correct.

Re: [PATCH V2 0/4] string list functions

[Markus Armbruster]

Steve Sistare <steven.sistare@oracle.com> writes:

> Add some handy string list functions, for general use now, and for
> eventual use in the cpr/live update patches.

Submit them together with uses, please.

Re: [PATCH V2 0/4] string list functions

[Daniel P. Berrangé]

On Tue, Feb 07, 2023 at 10:48:43AM -0800, Steve Sistare wrote:
> Add some handy string list functions, for general use now, and for
> eventual use in the cpr/live update patches.
> 
> Steve Sistare (4):
>   qapi: strList_from_string
>   qapi: QAPI_LIST_LENGTH
>   qapi: strv_from_strList
>   qapi: strList unit tests

I know that the 'strList' type falls out naturally from the
QAPI type generator for arrays, but I've always considered
it to be a rather awkward result.  The normal C approach
would be to use 'char **' NULL terminated, which conveniently
already has a bunch of helper APIs from glib, and is also
accepted or returned by various other functions we might
like to use.

Should we consider making the QAPI generator handle string
lists as a special case, emitting 'char **' instead of this
series ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH V2 1/4] qapi: strList_from_string

[Steven Sistare]

On 2/9/2023 5:02 AM, Markus Armbruster wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
>> Steven Sistare <steven.sistare@oracle.com> writes:
>>
>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>>>> Hi
>>>>
>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>>
>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>>>> as strList_from_string(), and move it to qapi/util.c.
>>>>>
>>>>> No functional change.
>>>>
>>>> The g_strsplit() version was a bit simpler, but if you want to
>>>> optimize it a bit for 1 char delimiter only, ok.
>>>>
>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Yes, and it saves a malloc+free for the array.  Small stuff, but I
>>> thought it worth a few lines of code.  Thanks for the speedy review!
>>
>> But is the HMP path that performance critical? Otherwise I'd favour
>> consistent use of the glib APIs because its one less thing to get wrong.
> 
> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
> strlist_from_comma_list() as hmp_split_at_comma()", with a different
> function name and place, and an additional parameter.
> 
> There is no explanation for the revert.
> 
> An intentional revert without even mentioning it would be uncourteous.
> I don't think this is the case here.  I figure you wrote this patch
> before you saw my commit, then rebased, keeping the old code.  A simple
> rebase mistake, easy enough to correct.

Hi Markus, I am sorry, I intended no slight.  I will document your commit
in this commit message.  And in response to Alex' comment, I will use your
version as the basis of the new function.

For more context, this patch has been part of my larger series for live update,
and I am submitting this separately to reduce the size of that series and make
forward progress:
    https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/

In that series, strList_from_string is used to parse a space-separated list of args
in an HMP command, and pass them to the new qemu binary.
    https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/

I moved and renamed the generalized function because I thought it might be useful
to others in the future, along with the other functions in this 'string list functions'
patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
current location.

- Steve

Re: [PATCH V2 0/4] string list functions

[Steven Sistare]

On 2/9/2023 5:05 AM, Markus Armbruster wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> Add some handy string list functions, for general use now, and for
>> eventual use in the cpr/live update patches.
> 
> Submit them together with uses, please.

Are you OK with me describing my intended uses in the commit message?  Or are you 
suggesting I go back to submitting these in the larger live update series?

- Steve

Re: [PATCH V2 0/4] string list functions

[Steven Sistare]

On 2/9/2023 5:48 AM, Daniel P. Berrangé wrote:
> On Tue, Feb 07, 2023 at 10:48:43AM -0800, Steve Sistare wrote:
>> Add some handy string list functions, for general use now, and for
>> eventual use in the cpr/live update patches.
>>
>> Steve Sistare (4):
>>   qapi: strList_from_string
>>   qapi: QAPI_LIST_LENGTH
>>   qapi: strv_from_strList
>>   qapi: strList unit tests
> 
> I know that the 'strList' type falls out naturally from the
> QAPI type generator for arrays, but I've always considered
> it to be a rather awkward result.  The normal C approach
> would be to use 'char **' NULL terminated, which conveniently
> already has a bunch of helper APIs from glib, and is also
> accepted or returned by various other functions we might
> like to use.
> 
> Should we consider making the QAPI generator handle string
> lists as a special case, emitting 'char **' instead of this
> series ?
> 
> With regards,
> Daniel

That is an intellectually appealing idea, but it sounds like a disproportionate 
effort to handle this small use case.  It would also make string list handling
be different than the other qapi lists: boolList, sizeList, uint64List, etc.

- Steve

Re: [PATCH V2 0/4] string list functions

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/9/2023 5:05 AM, Markus Armbruster wrote:
>> Steve Sistare <steven.sistare@oracle.com> writes:
>> 
>>> Add some handy string list functions, for general use now, and for
>>> eventual use in the cpr/live update patches.
>> 
>> Submit them together with uses, please.
>
> Are you OK with me describing my intended uses in the commit message?  Or are you 
> suggesting I go back to submitting these in the larger live update series?

The patches will be merged only together with actual uses.  Posting them
separately may make sense or not; use your judgement.  If you elect to
post separately, have the cover letters point to each other, please.

Re: [PATCH V2 1/4] qapi: strList_from_string

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/9/2023 5:02 AM, Markus Armbruster wrote:
>> Alex Bennée <alex.bennee@linaro.org> writes:
>> 
>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>
>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>>>>> Hi
>>>>>
>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>>>
>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>>>>> as strList_from_string(), and move it to qapi/util.c.
>>>>>>
>>>>>> No functional change.
>>>>>
>>>>> The g_strsplit() version was a bit simpler, but if you want to
>>>>> optimize it a bit for 1 char delimiter only, ok.
>>>>>
>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> Yes, and it saves a malloc+free for the array.  Small stuff, but I
>>>> thought it worth a few lines of code.  Thanks for the speedy review!
>>>
>>> But is the HMP path that performance critical? Otherwise I'd favour
>>> consistent use of the glib APIs because its one less thing to get wrong.
>> 
>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
>> strlist_from_comma_list() as hmp_split_at_comma()", with a different
>> function name and place, and an additional parameter.
>> 
>> There is no explanation for the revert.
>> 
>> An intentional revert without even mentioning it would be uncourteous.
>> I don't think this is the case here.  I figure you wrote this patch
>> before you saw my commit, then rebased, keeping the old code.  A simple
>> rebase mistake, easy enough to correct.
>
> Hi Markus, I am sorry, I intended no slight.

Certainly no offense taken :)

>                                               I will document your commit
> in this commit message.  And in response to Alex' comment, I will use your
> version as the basis of the new function.
>
> For more context, this patch has been part of my larger series for live update,
> and I am submitting this separately to reduce the size of that series and make
> forward progress:
>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>
> In that series, strList_from_string is used to parse a space-separated list of args
> in an HMP command, and pass them to the new qemu binary.
>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>
> I moved and renamed the generalized function because I thought it might be useful
> to others in the future, along with the other functions in this 'string list functions'
> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
> current location.

I'm fine with moving it out of monitor/ if there are uses outside the
monitor.  I just don't think qapi/ is the right home.

I'm also fine with generalizing the function to better serve new uses.

Re: [PATCH V2 1/4] qapi: strList_from_string

[Steven Sistare]

On 2/9/2023 11:46 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
> 
>> On 2/9/2023 5:02 AM, Markus Armbruster wrote:
>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>
>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>>
>>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>>>>>> Hi
>>>>>>
>>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>>>>
>>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>>>>>> as strList_from_string(), and move it to qapi/util.c.
>>>>>>>
>>>>>>> No functional change.
>>>>>>
>>>>>> The g_strsplit() version was a bit simpler, but if you want to
>>>>>> optimize it a bit for 1 char delimiter only, ok.
>>>>>>
>>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> Yes, and it saves a malloc+free for the array.  Small stuff, but I
>>>>> thought it worth a few lines of code.  Thanks for the speedy review!
>>>>
>>>> But is the HMP path that performance critical? Otherwise I'd favour
>>>> consistent use of the glib APIs because its one less thing to get wrong.
>>>
>>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
>>> strlist_from_comma_list() as hmp_split_at_comma()", with a different
>>> function name and place, and an additional parameter.
>>>
>>> There is no explanation for the revert.
>>>
>>> An intentional revert without even mentioning it would be uncourteous.
>>> I don't think this is the case here.  I figure you wrote this patch
>>> before you saw my commit, then rebased, keeping the old code.  A simple
>>> rebase mistake, easy enough to correct.
>>
>> Hi Markus, I am sorry, I intended no slight.
> 
> Certainly no offense taken :)
> 
>>                                               I will document your commit
>> in this commit message.  And in response to Alex' comment, I will use your
>> version as the basis of the new function.
>>
>> For more context, this patch has been part of my larger series for live update,
>> and I am submitting this separately to reduce the size of that series and make
>> forward progress:
>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>
>> In that series, strList_from_string is used to parse a space-separated list of args
>> in an HMP command, and pass them to the new qemu binary.
>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>
>> I moved and renamed the generalized function because I thought it might be useful
>> to others in the future, along with the other functions in this 'string list functions'
>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>> current location.
> 
> I'm fine with moving it out of monitor/ if there are uses outside the
> monitor.  I just don't think qapi/ is the right home.

I don't know where else it would go, as strList is a QAPI type.
include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
seems like the natural place to add qapi strList functions.  I am open to
suggestions.

- Steve

> 
> I'm also fine with generalizing the function to better serve new uses.
> 

Re: [PATCH V2 1/4] qapi: strList_from_string

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>> Steven Sistare <steven.sistare@oracle.com> writes:
>> 
>>> On 2/9/2023 5:02 AM, Markus Armbruster wrote:
>>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>>
>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>>>
>>>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>>>>>
>>>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>>>>>>> as strList_from_string(), and move it to qapi/util.c.
>>>>>>>>
>>>>>>>> No functional change.
>>>>>>>
>>>>>>> The g_strsplit() version was a bit simpler, but if you want to
>>>>>>> optimize it a bit for 1 char delimiter only, ok.
>>>>>>>
>>>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>
>>>>>> Yes, and it saves a malloc+free for the array.  Small stuff, but I
>>>>>> thought it worth a few lines of code.  Thanks for the speedy review!
>>>>>
>>>>> But is the HMP path that performance critical? Otherwise I'd favour
>>>>> consistent use of the glib APIs because its one less thing to get wrong.
>>>>
>>>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
>>>> strlist_from_comma_list() as hmp_split_at_comma()", with a different
>>>> function name and place, and an additional parameter.
>>>>
>>>> There is no explanation for the revert.
>>>>
>>>> An intentional revert without even mentioning it would be uncourteous.
>>>> I don't think this is the case here.  I figure you wrote this patch
>>>> before you saw my commit, then rebased, keeping the old code.  A simple
>>>> rebase mistake, easy enough to correct.
>>>
>>> Hi Markus, I am sorry, I intended no slight.
>> 
>> Certainly no offense taken :)
>> 
>>>                                               I will document your commit
>>> in this commit message.  And in response to Alex' comment, I will use your
>>> version as the basis of the new function.
>>>
>>> For more context, this patch has been part of my larger series for live update,
>>> and I am submitting this separately to reduce the size of that series and make
>>> forward progress:
>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>
>>> In that series, strList_from_string is used to parse a space-separated list of args
>>> in an HMP command, and pass them to the new qemu binary.
>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>
>>> I moved and renamed the generalized function because I thought it might be useful
>>> to others in the future, along with the other functions in this 'string list functions'
>>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>>> current location.
>> 
>> I'm fine with moving it out of monitor/ if there are uses outside the
>> monitor.  I just don't think qapi/ is the right home.
>
> I don't know where else it would go, as strList is a QAPI type.
> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
> seems like the natural place to add qapi strList functions.  I am open to
> suggestions.

What about util/?  Plenty of QAPI use there already.

Another thought.  Current hmp_split_at_comma() does two things:

    strList *hmp_split_at_comma(const char *str)
    {

One, split a comma-separated string into NULL-terminated a dynamically
allocated char *[]:

        char **split = g_strsplit(str ?: "", ",", -1);

Two, convert a dynamically allocated char *[] into a strList:

        strList *res = NULL;
        strList **tail = &res;
        int i;

        for (i = 0; split[i]; i++) {
            QAPI_LIST_APPEND(tail, split[i]);
        }

        g_free(split);
        return res;
    }

Part two could live in qapi/.

>
> - Steve
>
>> 
>> I'm also fine with generalizing the function to better serve new uses.
>> 

Re: [PATCH V2 1/4] qapi: strList_from_string

[Steven Sistare]

On 2/9/2023 1:59 PM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>
>>>> On 2/9/2023 5:02 AM, Markus Armbruster wrote:
>>>>> Alex Bennée <alex.bennee@linaro.org> writes:
>>>>>
>>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>>>>
>>>>>>> On 2/8/2023 1:43 AM, Marc-André Lureau wrote:
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Tue, Feb 7, 2023 at 10:50 PM Steve Sistare <steven.sistare@oracle.com> wrote:
>>>>>>>>>
>>>>>>>>> Generalize hmp_split_at_comma() to take any delimiter character, rename
>>>>>>>>> as strList_from_string(), and move it to qapi/util.c.
>>>>>>>>>
>>>>>>>>> No functional change.
>>>>>>>>
>>>>>>>> The g_strsplit() version was a bit simpler, but if you want to
>>>>>>>> optimize it a bit for 1 char delimiter only, ok.
>>>>>>>>
>>>>>>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> Yes, and it saves a malloc+free for the array.  Small stuff, but I
>>>>>>> thought it worth a few lines of code.  Thanks for the speedy review!
>>>>>>
>>>>>> But is the HMP path that performance critical? Otherwise I'd favour
>>>>>> consistent use of the glib APIs because its one less thing to get wrong.
>>>>>
>>>>> The patch reverts my recent commit 0d79271b570 "hmp: Rewrite
>>>>> strlist_from_comma_list() as hmp_split_at_comma()", with a different
>>>>> function name and place, and an additional parameter.
>>>>>
>>>>> There is no explanation for the revert.
>>>>>
>>>>> An intentional revert without even mentioning it would be uncourteous.
>>>>> I don't think this is the case here.  I figure you wrote this patch
>>>>> before you saw my commit, then rebased, keeping the old code.  A simple
>>>>> rebase mistake, easy enough to correct.
>>>>
>>>> Hi Markus, I am sorry, I intended no slight.
>>>
>>> Certainly no offense taken :)
>>>
>>>>                                               I will document your commit
>>>> in this commit message.  And in response to Alex' comment, I will use your
>>>> version as the basis of the new function.
>>>>
>>>> For more context, this patch has been part of my larger series for live update,
>>>> and I am submitting this separately to reduce the size of that series and make
>>>> forward progress:
>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>>
>>>> In that series, strList_from_string is used to parse a space-separated list of args
>>>> in an HMP command, and pass them to the new qemu binary.
>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>>
>>>> I moved and renamed the generalized function because I thought it might be useful
>>>> to others in the future, along with the other functions in this 'string list functions'
>>>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>>>> current location.
>>>
>>> I'm fine with moving it out of monitor/ if there are uses outside the
>>> monitor.  I just don't think qapi/ is the right home.
>>
>> I don't know where else it would go, as strList is a QAPI type.
>> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
>> seems like the natural place to add qapi strList functions.  I am open to
>> suggestions.
> 
> What about util/?  Plenty of QAPI use there already.
> 
> Another thought.  Current hmp_split_at_comma() does two things:
> 
>     strList *hmp_split_at_comma(const char *str)
>     {
> 
> One, split a comma-separated string into NULL-terminated a dynamically
> allocated char *[]:
> 
>         char **split = g_strsplit(str ?: "", ",", -1);
> 
> Two, convert a dynamically allocated char *[] into a strList:
> 
>         strList *res = NULL;
>         strList **tail = &res;
>         int i;
> 
>         for (i = 0; split[i]; i++) {
>             QAPI_LIST_APPEND(tail, split[i]);
>         }
> 
>         g_free(split);
>         return res;
>     }
> 
> Part two could live in qapi/.

Works for me.

For future reference, what is your organizing principle for putting things in 
qapi/ vs util/ ?  I see plenty of calls to g_str* functions from qapi/*, so I 
don't know why removing g_strsplit changes the answer.

Per your principle, where does strv_from_strList (patch 3) belong?  And if I
substitute char ** for GStrv, does the answer change?

- Steve

Re: [PATCH V2 0/4] string list functions

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 07, 2023 at 10:48:43AM -0800, Steve Sistare wrote:
>> Add some handy string list functions, for general use now, and for
>> eventual use in the cpr/live update patches.
>> 
>> Steve Sistare (4):
>>   qapi: strList_from_string
>>   qapi: QAPI_LIST_LENGTH
>>   qapi: strv_from_strList
>>   qapi: strList unit tests
>
> I know that the 'strList' type falls out naturally from the
> QAPI type generator for arrays, but I've always considered
> it to be a rather awkward result.  The normal C approach
> would be to use 'char **' NULL terminated, which conveniently
> already has a bunch of helper APIs from glib, and is also
> accepted or returned by various other functions we might
> like to use.
>
> Should we consider making the QAPI generator handle string
> lists as a special case, emitting 'char **' instead of this
> series ?

I don't like special cases.  I also don't like GenericList in any case.

I believe a linked list was chosen because it results in a fairly simple
visitor interface and implementation.  But it's a poor data structure
for a homogeneous sequence that rarely if ever changes: lots of pointer
chasing, waste of memory when the elements are small.

Output visitors walk the sequence in order.  An array would be perfect.

Input visitors build the sequence by appending elements in order.
A flexible array like GArray would do.

I'm not aware of other code mutating GenericLists or its descendants.
It might exist.  I'd be surprised if there's much of it, though.

Re: [PATCH V2 1/4] qapi: strList_from_string

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/9/2023 1:59 PM, Markus Armbruster wrote:
>> Steven Sistare <steven.sistare@oracle.com> writes:
>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>>> Steven Sistare <steven.sistare@oracle.com> writes:

[...]

>>>>> For more context, this patch has been part of my larger series for live update,
>>>>> and I am submitting this separately to reduce the size of that series and make
>>>>> forward progress:
>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>>>
>>>>> In that series, strList_from_string is used to parse a space-separated list of args
>>>>> in an HMP command, and pass them to the new qemu binary.
>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>>>
>>>>> I moved and renamed the generalized function because I thought it might be useful
>>>>> to others in the future, along with the other functions in this 'string list functions'
>>>>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>>>>> current location.
>>>>
>>>> I'm fine with moving it out of monitor/ if there are uses outside the
>>>> monitor.  I just don't think qapi/ is the right home.
>>>
>>> I don't know where else it would go, as strList is a QAPI type.
>>> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
>>> seems like the natural place to add qapi strList functions.  I am open to
>>> suggestions.
>> 
>> What about util/?  Plenty of QAPI use there already.
>> 
>> Another thought.  Current hmp_split_at_comma() does two things:
>> 
>>     strList *hmp_split_at_comma(const char *str)
>>     {
>> 
>> One, split a comma-separated string into NULL-terminated a dynamically
>> allocated char *[]:
>> 
>>         char **split = g_strsplit(str ?: "", ",", -1);
>> 
>> Two, convert a dynamically allocated char *[] into a strList:
>> 
>>         strList *res = NULL;
>>         strList **tail = &res;
>>         int i;
>> 
>>         for (i = 0; split[i]; i++) {
>>             QAPI_LIST_APPEND(tail, split[i]);
>>         }
>> 
>>         g_free(split);
>>         return res;
>>     }
>> 
>> Part two could live in qapi/.
>
> Works for me.

Note that I'm not demanding such a split.  I'm merely throwing in
another idea for you to use or reject.

> For future reference, what is your organizing principle for putting things in 
> qapi/ vs util/ ?  I see plenty of calls to g_str* functions from qapi/*, so I 
> don't know why removing g_strsplit changes the answer.
>
> Per your principle, where does strv_from_strList (patch 3) belong?  And if I
> substitute char ** for GStrv, does the answer change?

As is, qapi/qapi-util provides:

1. Helpers for qapi/ and QAPI-generated code.  Some of them are
   used elsewhere, too.  That's fine.

2. Tools for working with QAPI data types such as GenericList.

strv_from_strList() would fall under 2.  Same if you use char **
instead.

hmp_split_at_comma() admittedly also falls under 2.  I just dislike
putting things under qapi/ that contradict QAPI design principles.

util/ is a bit of a grabbag, I feel.  Perhaps we could describe it as
"utilities that don't really fit into a particular subsystem".

Does this help you along?

Re: [PATCH V2 1/4] qapi: strList_from_string

[Steven Sistare]

On 2/10/2023 4:25 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
> 
>> On 2/9/2023 1:59 PM, Markus Armbruster wrote:
>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
> 
> [...]
> 
>>>>>> For more context, this patch has been part of my larger series for live update,
>>>>>> and I am submitting this separately to reduce the size of that series and make
>>>>>> forward progress:
>>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>>>>
>>>>>> In that series, strList_from_string is used to parse a space-separated list of args
>>>>>> in an HMP command, and pass them to the new qemu binary.
>>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>>>>
>>>>>> I moved and renamed the generalized function because I thought it might be useful
>>>>>> to others in the future, along with the other functions in this 'string list functions'
>>>>>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>>>>>> current location.
>>>>>
>>>>> I'm fine with moving it out of monitor/ if there are uses outside the
>>>>> monitor.  I just don't think qapi/ is the right home.
>>>>
>>>> I don't know where else it would go, as strList is a QAPI type.
>>>> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
>>>> seems like the natural place to add qapi strList functions.  I am open to
>>>> suggestions.
>>>
>>> What about util/?  Plenty of QAPI use there already.
>>>
>>> Another thought.  Current hmp_split_at_comma() does two things:
>>>
>>>     strList *hmp_split_at_comma(const char *str)
>>>     {
>>>
>>> One, split a comma-separated string into NULL-terminated a dynamically
>>> allocated char *[]:
>>>
>>>         char **split = g_strsplit(str ?: "", ",", -1);
>>>
>>> Two, convert a dynamically allocated char *[] into a strList:
>>>
>>>         strList *res = NULL;
>>>         strList **tail = &res;
>>>         int i;
>>>
>>>         for (i = 0; split[i]; i++) {
>>>             QAPI_LIST_APPEND(tail, split[i]);
>>>         }
>>>
>>>         g_free(split);
>>>         return res;
>>>     }
>>>
>>> Part two could live in qapi/.
>>
>> Works for me.
> 
> Note that I'm not demanding such a split.  I'm merely throwing in
> another idea for you to use or reject.

I decided to not split the function.  IMO having part 2 free memory allocated
by its caller is not clean.

However, I will base it on your original function, slightly modified:

strList *strList_from_string(const char *str, char *delim)
{
    g_autofree char **split = g_strsplit(str ?: "", delim, -1);
    strList *res = NULL;
    strList **tail = &res;

    for (; *split; split++) {
        QAPI_LIST_APPEND(tail, *split);
    }

    return res;
}

>> For future reference, what is your organizing principle for putting things in 
>> qapi/ vs util/ ?  I see plenty of calls to g_str* functions from qapi/*, so I 
>> don't know why removing g_strsplit changes the answer.
>>
>> Per your principle, where does strv_from_strList (patch 3) belong?  And if I
>> substitute char ** for GStrv, does the answer change?
> 
> As is, qapi/qapi-util provides:
> 
> 1. Helpers for qapi/ and QAPI-generated code.  Some of them are
>    used elsewhere, too.  That's fine.
> 
> 2. Tools for working with QAPI data types such as GenericList.
> 
> strv_from_strList() would fall under 2.  Same if you use char **
> instead.
> 
> hmp_split_at_comma() admittedly also falls under 2.  I just dislike
> putting things under qapi/ that contradict QAPI design principles.

What design principle does strList_from_string contradict?  Are you OK with 
putting the simplified version shown above in qapi-util?

(and apologies for my long delay in continuing this conversation).

- Steve

> 
> util/ is a bit of a grabbag, I feel.  Perhaps we could describe it as
> "utilities that don't really fit into a particular subsystem".
> 
> Does this help you along?
> 

Re: [PATCH V2 1/4] qapi: strList_from_string

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 2/10/2023 4:25 AM, Markus Armbruster wrote:
>> Steven Sistare <steven.sistare@oracle.com> writes:
>> 
>>> On 2/9/2023 1:59 PM, Markus Armbruster wrote:
>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>> 
>> [...]
>> 
>>>>>>> For more context, this patch has been part of my larger series for live update,
>>>>>>> and I am submitting this separately to reduce the size of that series and make
>>>>>>> forward progress:
>>>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>>>>>
>>>>>>> In that series, strList_from_string is used to parse a space-separated list of args
>>>>>>> in an HMP command, and pass them to the new qemu binary.
>>>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>>>>>
>>>>>>> I moved and renamed the generalized function because I thought it might be useful
>>>>>>> to others in the future, along with the other functions in this 'string list functions'
>>>>>>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>>>>>>> current location.
>>>>>>
>>>>>> I'm fine with moving it out of monitor/ if there are uses outside the
>>>>>> monitor.  I just don't think qapi/ is the right home.
>>>>>
>>>>> I don't know where else it would go, as strList is a QAPI type.
>>>>> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
>>>>> seems like the natural place to add qapi strList functions.  I am open to
>>>>> suggestions.
>>>>
>>>> What about util/?  Plenty of QAPI use there already.
>>>>
>>>> Another thought.  Current hmp_split_at_comma() does two things:
>>>>
>>>>     strList *hmp_split_at_comma(const char *str)
>>>>     {
>>>>
>>>> One, split a comma-separated string into NULL-terminated a dynamically
>>>> allocated char *[]:
>>>>
>>>>         char **split = g_strsplit(str ?: "", ",", -1);
>>>>
>>>> Two, convert a dynamically allocated char *[] into a strList:
>>>>
>>>>         strList *res = NULL;
>>>>         strList **tail = &res;
>>>>         int i;
>>>>
>>>>         for (i = 0; split[i]; i++) {
>>>>             QAPI_LIST_APPEND(tail, split[i]);
>>>>         }
>>>>
>>>>         g_free(split);
>>>>         return res;
>>>>     }
>>>>
>>>> Part two could live in qapi/.
>>>
>>> Works for me.
>> 
>> Note that I'm not demanding such a split.  I'm merely throwing in
>> another idea for you to use or reject.
>
> I decided to not split the function.  IMO having part 2 free memory allocated
> by its caller is not clean.
>
> However, I will base it on your original function, slightly modified:
>
> strList *strList_from_string(const char *str, char *delim)
> {
>     g_autofree char **split = g_strsplit(str ?: "", delim, -1);
>     strList *res = NULL;
>     strList **tail = &res;
>
>     for (; *split; split++) {
>         QAPI_LIST_APPEND(tail, *split);
>     }
>
>     return res;
> }
>
>>> For future reference, what is your organizing principle for putting things in 
>>> qapi/ vs util/ ?  I see plenty of calls to g_str* functions from qapi/*, so I 
>>> don't know why removing g_strsplit changes the answer.
>>>
>>> Per your principle, where does strv_from_strList (patch 3) belong?  And if I
>>> substitute char ** for GStrv, does the answer change?
>> 
>> As is, qapi/qapi-util provides:
>> 
>> 1. Helpers for qapi/ and QAPI-generated code.  Some of them are
>>    used elsewhere, too.  That's fine.
>> 
>> 2. Tools for working with QAPI data types such as GenericList.
>> 
>> strv_from_strList() would fall under 2.  Same if you use char **
>> instead.
>> 
>> hmp_split_at_comma() admittedly also falls under 2.  I just dislike
>> putting things under qapi/ that contradict QAPI design principles.
>
> What design principle does strList_from_string contradict?  Are you OK with 
> putting the simplified version shown above in qapi-util?

The design principle is "use JSON to encode structured data as text in
QAPI/QMP".

Do: "mumble": [1, 2, 3]

Don't: "mumble": "1,2,3"

We violate the principle in a couple of places.  Some are arguably
mistakes, some are pragmatic exceptions.

The principle implies "the only parser QAPI needs is the JSON parser".

By adding other parsers to QAPI, we send a misleading signal, namely
that encoding structured data in a way that requires parsing is okay.
It's not, generally.

So, I'd prefer to find another home for code that splits strings at
comma / delimiter.

> (and apologies for my long delay in continuing this conversation).

I'm in no position to take offense there ;)

> - Steve
>
>> 
>> util/ is a bit of a grabbag, I feel.  Perhaps we could describe it as
>> "utilities that don't really fit into a particular subsystem".
>> 
>> Does this help you along?

Re: [PATCH V2 1/4] qapi: strList_from_string

[Steven Sistare]

On 6/13/2023 8:33 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
>> On 2/10/2023 4:25 AM, Markus Armbruster wrote:
>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>> On 2/9/2023 1:59 PM, Markus Armbruster wrote:
>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>>>> On 2/9/2023 11:46 AM, Markus Armbruster wrote:
>>>>>>> Steven Sistare <steven.sistare@oracle.com> writes:
>>>
>>> [...]
>>>
>>>>>>>> For more context, this patch has been part of my larger series for live update,
>>>>>>>> and I am submitting this separately to reduce the size of that series and make
>>>>>>>> forward progress:
>>>>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-1-git-send-email-steven.sistare@oracle.com/
>>>>>>>>
>>>>>>>> In that series, strList_from_string is used to parse a space-separated list of args
>>>>>>>> in an HMP command, and pass them to the new qemu binary.
>>>>>>>>     https://lore.kernel.org/qemu-devel/1658851843-236870-16-git-send-email-steven.sistare@oracle.com/
>>>>>>>>
>>>>>>>> I moved and renamed the generalized function because I thought it might be useful
>>>>>>>> to others in the future, along with the other functions in this 'string list functions'
>>>>>>>> patch series.  But if you disagree, I can minimally modify hmp_split_at_comma() in its 
>>>>>>>> current location.
>>>>>>>
>>>>>>> I'm fine with moving it out of monitor/ if there are uses outside the
>>>>>>> monitor.  I just don't think qapi/ is the right home.
>>>>>>
>>>>>> I don't know where else it would go, as strList is a QAPI type.
>>>>>> include/qapi/util.h already defines QAPI_LIST_PREPEND and QAPI_LIST_APPEND, so it
>>>>>> seems like the natural place to add qapi strList functions.  I am open to
>>>>>> suggestions.
>>>>>
>>>>> What about util/?  Plenty of QAPI use there already.
>>>>>
>>>>> Another thought.  Current hmp_split_at_comma() does two things:
>>>>>
>>>>>     strList *hmp_split_at_comma(const char *str)
>>>>>     {
>>>>>
>>>>> One, split a comma-separated string into NULL-terminated a dynamically
>>>>> allocated char *[]:
>>>>>
>>>>>         char **split = g_strsplit(str ?: "", ",", -1);
>>>>>
>>>>> Two, convert a dynamically allocated char *[] into a strList:
>>>>>
>>>>>         strList *res = NULL;
>>>>>         strList **tail = &res;
>>>>>         int i;
>>>>>
>>>>>         for (i = 0; split[i]; i++) {
>>>>>             QAPI_LIST_APPEND(tail, split[i]);
>>>>>         }
>>>>>
>>>>>         g_free(split);
>>>>>         return res;
>>>>>     }
>>>>>
>>>>> Part two could live in qapi/.
>>>>
>>>> Works for me.
>>>
>>> Note that I'm not demanding such a split.  I'm merely throwing in
>>> another idea for you to use or reject.
>>
>> I decided to not split the function.  IMO having part 2 free memory allocated
>> by its caller is not clean.
>>
>> However, I will base it on your original function, slightly modified:
>>
>> strList *strList_from_string(const char *str, char *delim)
>> {
>>     g_autofree char **split = g_strsplit(str ?: "", delim, -1);
>>     strList *res = NULL;
>>     strList **tail = &res;
>>
>>     for (; *split; split++) {
>>         QAPI_LIST_APPEND(tail, *split);
>>     }
>>
>>     return res;
>> }
>>
>>>> For future reference, what is your organizing principle for putting things in 
>>>> qapi/ vs util/ ?  I see plenty of calls to g_str* functions from qapi/*, so I 
>>>> don't know why removing g_strsplit changes the answer.
>>>>
>>>> Per your principle, where does strv_from_strList (patch 3) belong?  And if I
>>>> substitute char ** for GStrv, does the answer change?
>>>
>>> As is, qapi/qapi-util provides:
>>>
>>> 1. Helpers for qapi/ and QAPI-generated code.  Some of them are
>>>    used elsewhere, too.  That's fine.
>>>
>>> 2. Tools for working with QAPI data types such as GenericList.
>>>
>>> strv_from_strList() would fall under 2.  Same if you use char **
>>> instead.
>>>
>>> hmp_split_at_comma() admittedly also falls under 2.  I just dislike
>>> putting things under qapi/ that contradict QAPI design principles.
>>
>> What design principle does strList_from_string contradict?  Are you OK with 
>> putting the simplified version shown above in qapi-util?
> 
> The design principle is "use JSON to encode structured data as text in
> QAPI/QMP".
> 
> Do: "mumble": [1, 2, 3]
> 
> Don't: "mumble": "1,2,3"

I don't mumble, but I sometimes mutter and ramble.

> We violate the principle in a couple of places.  Some are arguably
> mistakes, some are pragmatic exceptions.
> 
> The principle implies "the only parser QAPI needs is the JSON parser".
> 
> By adding other parsers to QAPI, we send a misleading signal, namely
> that encoding structured data in a way that requires parsing is okay.
> It's not, generally.
> 
> So, I'd prefer to find another home for code that splits strings at
> comma / delimiter.
> 
>> (and apologies for my long delay in continuing this conversation).
> 
> I'm in no position to take offense there ;)

Thanks, that makes it clear.

I propose to move strList_from_string and strv_from_strList to new files
util/strList.c and include/qemu/strList.h, and leave QAPI_LIST_LENGTH in 
include/qapi/util.h.

(cutil.c already has string functions, but only uses simple C types, so
not the best place to add the strList type).

Sound OK?

- Steve

Re: [PATCH V2 1/4] qapi: strList_from_string

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 6/13/2023 8:33 AM, Markus Armbruster wrote:
>> Steven Sistare <steven.sistare@oracle.com> writes:

[...]

>>> What design principle does strList_from_string contradict?  Are you OK with 
>>> putting the simplified version shown above in qapi-util?
>> 
>> The design principle is "use JSON to encode structured data as text in
>> QAPI/QMP".
>> 
>> Do: "mumble": [1, 2, 3]
>> 
>> Don't: "mumble": "1,2,3"
>
> I don't mumble, but I sometimes mutter and ramble.

Hah!

>> We violate the principle in a couple of places.  Some are arguably
>> mistakes, some are pragmatic exceptions.
>> 
>> The principle implies "the only parser QAPI needs is the JSON parser".
>> 
>> By adding other parsers to QAPI, we send a misleading signal, namely
>> that encoding structured data in a way that requires parsing is okay.
>> It's not, generally.
>> 
>> So, I'd prefer to find another home for code that splits strings at
>> comma / delimiter.
>> 
>>> (and apologies for my long delay in continuing this conversation).
>> 
>> I'm in no position to take offense there ;)
>
> Thanks, that makes it clear.
>
> I propose to move strList_from_string and strv_from_strList to new files
> util/strList.c and include/qemu/strList.h, and leave QAPI_LIST_LENGTH in 
> include/qapi/util.h.
>
> (cutil.c already has string functions, but only uses simple C types, so
> not the best place to add the strList type).
>
> Sound OK?

Works for me.  Thanks!