| Message ID | 1705099758-211963-6-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | string list functions | expand |
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; > }
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> Reviewed-by: Fabiano Rosas <farosas@suse.de>
Fabiano Rosas <farosas@suse.de> writes: > 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> > > Reviewed-by: Fabiano Rosas <farosas@suse.de> You'll have to reintroduce the qemu/cutils.h include: ../migration/exec.c: In function 'exec_get_cmd_path': ../migration/exec.c:37:5: error: implicit declaration of function 'pstrcat'; did you mean 'strcat'? [-Werror=implicit-function-declaration] 37 | pstrcat(detected_path, MAX_PATH, "\\cmd.exe"); | ^~~~~~~ | strcat ../migration/exec.c:37:5: error: nested extern declaration of 'pstrcat' [-Werror=nested-externs]
On 2/21/2024 10:54 AM, Fabiano Rosas wrote: > Fabiano Rosas <farosas@suse.de> writes: > >> 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> >> >> Reviewed-by: Fabiano Rosas <farosas@suse.de> > > You'll have to reintroduce the qemu/cutils.h include: > > ../migration/exec.c: In function 'exec_get_cmd_path': > ../migration/exec.c:37:5: error: implicit declaration of function 'pstrcat'; did you mean 'strcat'? [-Werror=implicit-function-declaration] > 37 | pstrcat(detected_path, MAX_PATH, "\\cmd.exe"); > | ^~~~~~~ > | strcat > ../migration/exec.c:37:5: error: nested extern declaration of 'pstrcat' [-Werror=nested-externs] Thanks, I will rebase to the tip and verify all is well before I post V5. - Steve
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; }
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(-)