[PATCH v7 0/3] string list functions

[PATCH v7 0/3] string list functions

[Markus Armbruster]

This is Steve's work (v1 to v5), tweaked by Philippe (v6), and now by
me.

v7:
* Old PATCH 1 dropped
  The proposed str_split() is a composition of g_strsplit() and a
  conversion from char ** to strList *.  I'm willing to accept a
  conversion function str_list_from_strv() next to
  strv_from_str_list().  I doubt having a function for the composition
  is worth the trouble.
* Old PATCH 4 dropped
  The tests mostly test g_strsplit().  I'm willing to accept focused
  tests for strv_from_str_list() and str_list_from_strv().
* PATCH 1-3: Commit messages tweaked
* PATCH 2: strv_from_strList() renamed strv_from_str_list(), and moved
  to qapi/qapi-type-helpers.c.  Function comment tweaked.  Local
  variable @argv renamed to @strv.
* PATCH 3: Adjust for the rename.

Steve Sistare (3):
  qapi: New QAPI_LIST_LENGTH()
  qapi: New strv_from_str_list()
  migration: simplify exec migration functions

 include/qapi/type-helpers.h |  8 ++++++
 include/qapi/util.h         | 13 +++++++++
 migration/exec.c            | 57 ++++++-------------------------------
 qapi/qapi-type-helpers.c    | 14 +++++++++
 4 files changed, 43 insertions(+), 49 deletions(-)

-- 
2.43.0

[PATCH v7 1/3] qapi: New QAPI_LIST_LENGTH()

[Markus Armbruster]

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

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 81a2b13a33..20dfea8a54 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -56,4 +56,17 @@ int parse_qapi_name(const char *name, bool complete);
     (tail) = &(*(tail))->next; \
 } while (0)
 
+/*
+ * For any GenericList @list, return its length.
+ */
+#define QAPI_LIST_LENGTH(list)                                      \
+    ({                                                              \
+        size_t _len = 0;                                            \
+        typeof(list) _tail;                                         \
+        for (_tail = list; _tail != NULL; _tail = _tail->next) {    \
+            _len++;                                                 \
+        }                                                           \
+        _len;                                                       \
+    })
+
 #endif
-- 
2.43.0

Re: [PATCH v7 1/3] qapi: New QAPI_LIST_LENGTH()

[Philippe Mathieu-Daudé]

On 27/2/24 16:33, Markus Armbruster wrote:
> From: Steve Sistare <steven.sistare@oracle.com>
> 
> 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(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v7 2/3] qapi: New strv_from_str_list()

[Markus Armbruster]

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

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

diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
index be1f181526..fc8352cdec 100644
--- a/include/qapi/type-helpers.h
+++ b/include/qapi/type-helpers.h
@@ -12,3 +12,11 @@
 #include "qapi/qapi-types-common.h"
 
 HumanReadableText *human_readable_text_from_str(GString *str);
+
+/*
+ * Produce and return a NULL-terminated array of strings from @list.
+ * The result is g_malloc()'d and all strings are g_strdup()'d.  It
+ * can be freed with g_strfreev(), or by g_auto(GStrv) automatic
+ * cleanup.
+ */
+char **strv_from_str_list(const strList *list);
diff --git a/qapi/qapi-type-helpers.c b/qapi/qapi-type-helpers.c
index f76b34f647..266da013ad 100644
--- a/qapi/qapi-type-helpers.c
+++ b/qapi/qapi-type-helpers.c
@@ -21,3 +21,17 @@ HumanReadableText *human_readable_text_from_str(GString *str)
 
     return ret;
 }
+
+char **strv_from_str_list(const strList *list)
+{
+    const strList *tail;
+    int i = 0;
+    char **strv = g_new(char *, QAPI_LIST_LENGTH(list) + 1);
+
+    for (tail = list; tail != NULL; tail = tail->next) {
+        strv[i++] = g_strdup(tail->value);
+    }
+    strv[i] = NULL;
+
+    return strv;
+}
-- 
2.43.0

Re: [PATCH v7 2/3] qapi: New strv_from_str_list()

[Philippe Mathieu-Daudé]

On 27/2/24 16:33, Markus Armbruster wrote:
> From: Steve Sistare <steven.sistare@oracle.com>
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/qapi/type-helpers.h |  8 ++++++++
>   qapi/qapi-type-helpers.c    | 14 ++++++++++++++
>   2 files changed, 22 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v7 3/3] migration: simplify exec migration functions

[Markus Armbruster]

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

Simplify the exec migration code by using list utility functions.

As a side effect, this also fixes a minor memory leak.  On function return,
"g_auto(GStrv) argv" frees argv and each element, which is wrong, because
the function does not own the individual elements.  To compensate, the code
uses g_steal_pointer which NULLs argv and prevents the destructor from
running, but argv is leaked.

Fixes: cbab4face57b ("migration: convert exec backend ...")
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 migration/exec.c | 57 +++++++-----------------------------------------
 1 file changed, 8 insertions(+), 49 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 47d2f3b8fb..20e6cccf8c 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/type-helpers.h"
 #include "qemu/error-report.h"
 #include "channel.h"
 #include "exec.h"
@@ -39,51 +40,16 @@ const char *exec_get_cmd_path(void)
 }
 #endif
 
-/* provides the length of strList */
-static int
-str_list_length(strList *list)
-{
-    int len = 0;
-    strList *elem;
-
-    for (elem = list; elem != NULL; elem = elem->next) {
-        len++;
-    }
-
-    return len;
-}
-
-static void
-init_exec_array(strList *command, char **argv, Error **errp)
-{
-    int i = 0;
-    strList *lst;
-
-    for (lst = command; lst; lst = lst->next) {
-        argv[i++] = lst->value;
-    }
-
-    argv[i] = NULL;
-    return;
-}
-
 void exec_start_outgoing_migration(MigrationState *s, strList *command,
                                    Error **errp)
 {
-    QIOChannel *ioc;
-
-    int length = str_list_length(command);
-    g_auto(GStrv) argv = (char **) g_new0(const char *, length + 1);
-
-    init_exec_array(command, argv, errp);
+    QIOChannel *ioc = NULL;
+    g_auto(GStrv) argv = strv_from_str_list(command);
+    const char * const *args = (const char * const *) argv;
     g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
 
     trace_migration_exec_outgoing(new_command);
-    ioc = QIO_CHANNEL(
-        qio_channel_command_new_spawn(
-                            (const char * const *) g_steal_pointer(&argv),
-                            O_RDWR,
-                            errp));
+    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(args, O_RDWR, errp));
     if (!ioc) {
         return;
     }
@@ -105,19 +71,12 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
 void exec_start_incoming_migration(strList *command, Error **errp)
 {
     QIOChannel *ioc;
-
-    int length = str_list_length(command);
-    g_auto(GStrv) argv = (char **) g_new0(const char *, length + 1);
-
-    init_exec_array(command, argv, errp);
+    g_auto(GStrv) argv = strv_from_str_list(command);
+    const char * const *args = (const char * const *) argv;
     g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
 
     trace_migration_exec_incoming(new_command);
-    ioc = QIO_CHANNEL(
-        qio_channel_command_new_spawn(
-                            (const char * const *) g_steal_pointer(&argv),
-                            O_RDWR,
-                            errp));
+    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(args, O_RDWR, errp));
     if (!ioc) {
         return;
     }
-- 
2.43.0

Re: [PATCH v7 0/3] string list functions

[Steven Sistare]

All the changes look good - steve

On 2/27/2024 10:33 AM, Markus Armbruster wrote:
> This is Steve's work (v1 to v5), tweaked by Philippe (v6), and now by
> me.
> 
> v7:
> * Old PATCH 1 dropped
>   The proposed str_split() is a composition of g_strsplit() and a
>   conversion from char ** to strList *.  I'm willing to accept a
>   conversion function str_list_from_strv() next to
>   strv_from_str_list().  I doubt having a function for the composition
>   is worth the trouble.
> * Old PATCH 4 dropped
>   The tests mostly test g_strsplit().  I'm willing to accept focused
>   tests for strv_from_str_list() and str_list_from_strv().
> * PATCH 1-3: Commit messages tweaked
> * PATCH 2: strv_from_strList() renamed strv_from_str_list(), and moved
>   to qapi/qapi-type-helpers.c.  Function comment tweaked.  Local
>   variable @argv renamed to @strv.
> * PATCH 3: Adjust for the rename.
> 
> Steve Sistare (3):
>   qapi: New QAPI_LIST_LENGTH()
>   qapi: New strv_from_str_list()
>   migration: simplify exec migration functions
> 
>  include/qapi/type-helpers.h |  8 ++++++
>  include/qapi/util.h         | 13 +++++++++
>  migration/exec.c            | 57 ++++++-------------------------------
>  qapi/qapi-type-helpers.c    | 14 +++++++++
>  4 files changed, 43 insertions(+), 49 deletions(-)
> 

Re: [PATCH v7 0/3] string list functions

[Markus Armbruster]

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

> All the changes look good - steve

Thanks!

I can stick this into my next PR, ETA early next week.