Re: [PATCH v4 5/8] migration: converts exec backend to accept MigrateAddress struct.

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, May 12, 2023 at 02:32:37PM +0000, Het Gala wrote:
> Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> new wire protocol of MigrateAddress struct.
> 
> It is achived by parsing 'uri' string and storing migration parameters
> required for exec connection into strList struct.
> 
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/exec.c      | 58 ++++++++++++++++++++++++++++++++-----------
>  migration/exec.h      |  4 +--
>  migration/migration.c | 10 +++-----
>  3 files changed, 50 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/exec.c b/migration/exec.c
> index c4a3293246..210f4e9400 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -39,22 +39,51 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command,
> +/* 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, const 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;
>  
> -#ifdef WIN32
> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> +    int length = str_list_length(command);
> +    const char **argv = g_malloc0(length * sizeof(const char *));

g_malloc0 is almost never desirable to use, instead:

        g_new0(const char *, length);

>  
> -    trace_migration_exec_outgoing(command);
> +    init_exec_array(command, argv, errp);
> +    char *new_command = g_strjoinv(" ", (char **)argv);

Never freed - use

   g_autofree char *new_command...
   
> +    trace_migration_exec_outgoing(new_command);


>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>                                                      O_RDWR,
>                                                      errp));
>      if (!ioc) {
> +        g_free(argv);
>          return;
>      }

argv needs freeing in success too. Simpler to declare it
with

    g_auto(GStrv) argv = .....



>  
> @@ -72,21 +101,22 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
>      return G_SOURCE_REMOVE;
>  }
>  
> -void exec_start_incoming_migration(const char *command, Error **errp)
> +void exec_start_incoming_migration(strList *command, Error **errp)
>  {
>      QIOChannel *ioc;
>  
> -#ifdef WIN32
> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> +    int length = str_list_length(command);
> +    const char **argv = g_malloc0(length * sizeof(const char *));
> +
> +    init_exec_array(command, argv, errp);
> +    char *new_command = g_strjoinv(" ", (char **)argv);
>  
> -    trace_migration_exec_incoming(command);
> +    trace_migration_exec_incoming(new_command);
>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
>                                                      O_RDWR,
>                                                      errp));
>      if (!ioc) {
> +        g_free(argv);
>          return;
>      }

All the same comments as the outgoing case.

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 :|