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;
}
--
1.8.3.1Steve 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
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;
> }
© 2016 - 2024 Red Hat, Inc.