| Message ID | 20230512143240.192504-6-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Modified 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
Het Gala <het.gala@nutanix.com> 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; > +} I can't believe tat we have a list type and we don't have a length() function for that type. Sniff. Reviewed-by: Juan Quintela <quintela@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
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
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 *)); - trace_migration_exec_outgoing(command); + init_exec_array(command, argv, errp); + char *new_command = g_strjoinv(" ", (char **)argv); + + trace_migration_exec_outgoing(new_command); ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv, O_RDWR, errp)); if (!ioc) { + g_free(argv); return; } @@ -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; } diff --git a/migration/exec.h b/migration/exec.h index 736cd71028..3107f205e3 100644 --- a/migration/exec.h +++ b/migration/exec.h @@ -23,8 +23,8 @@ #ifdef WIN32 const char *exec_get_cmd_path(void); #endif -void exec_start_incoming_migration(const char *host_port, Error **errp); +void exec_start_incoming_migration(strList *host_port, Error **errp); -void exec_start_outgoing_migration(MigrationState *s, const char *host_port, +void exec_start_outgoing_migration(MigrationState *s, strList *host_port, Error **errp); #endif diff --git a/migration/migration.c b/migration/migration.c index b7c3b939d5..6abd69df8d 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -455,7 +455,6 @@ static bool migrate_uri_parse(const char *uri, static void qemu_start_incoming_migration(const char *uri, Error **errp) { Error *local_err = NULL; - const char *p = NULL; MigrateAddress *channel = g_new0(MigrateAddress, 1); SocketAddress *saddr; @@ -483,8 +482,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp) } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) { rdma_start_incoming_migration(&channel->u.rdma, &local_err); #endif - } else if (strstart(uri, "exec:", &p)) { - exec_start_incoming_migration(p, errp); + } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) { + exec_start_incoming_migration(channel->u.exec.args, &local_err); } else { error_setg(errp, "unknown migration protocol: %s", uri); } @@ -1701,7 +1700,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, { Error *local_err = NULL; MigrationState *s = migrate_get_current(); - const char *p = NULL; MigrateAddress *channel = g_new0(MigrateAddress, 1); SocketAddress *saddr; @@ -1740,8 +1738,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk, } else if (channel->transport == MIGRATE_TRANSPORT_RDMA) { rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err); #endif - } else if (strstart(uri, "exec:", &p)) { - exec_start_outgoing_migration(s, p, &local_err); + } else if (channel->transport == MIGRATE_TRANSPORT_EXEC) { + exec_start_outgoing_migration(s, channel->u.exec.args, &local_err); } else { if (!(has_resume && resume)) { yank_unregister_instance(MIGRATION_YANK_INSTANCE);
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(-)