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

[Het Gala]

On 15/05/23 3:59 pm, Daniel P. Berrangé wrote:
> 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;
> > +}
> > +

Juan for your comment : "I can't believe tat we have a list type and we 
don't have a length() function for that type."

I had seen come patches regarding adding utility for finding length of a 
strList in util.h file as a MACRO, but I think the patches have still 
not gone in.

> > +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);
Ack. Will change that.
> >   
> > -    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...
Ack.
> > +    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 = .....
Yes, Ack.
> >   
> > @@ -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.
Ack. Thanks Daniel for the inputs.
> With regards,
> Daniel
Regards,
Het Gala