Re: [PATCH V4 5/5] migration: simplify exec migration functions

[Markus Armbruster <armbru@redhat.com>]

Het Gala, Peter, or Fabiano, please review.

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

> 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>
> ---
>  migration/exec.c | 58 ++++++++------------------------------------------------
>  1 file changed, 8 insertions(+), 50 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 47d2f3b..1312ca7 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -19,12 +19,12 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/error-report.h"
> +#include "qemu/strList.h"
>  #include "channel.h"
>  #include "exec.h"
>  #include "migration.h"
>  #include "io/channel-command.h"
>  #include "trace.h"
> -#include "qemu/cutils.h"
>  
>  #ifdef WIN32
>  const char *exec_get_cmd_path(void)
> @@ -39,51 +39,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_strList(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 +70,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_strList(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;
>      }