Het Gala <het.g...@nutanix.com> writes:
> This patch parses 'migrate' and 'migrate-incoming' QAPI's 'uri'
> string containing migration connection related information
> and stores them inside well defined 'MigrateAddress' struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnaka...@nutanix.com>
> Signed-off-by: Het Gala <het.g...@nutanix.com>
> Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
> migration/exec.c | 1 -
> migration/exec.h | 4 ++++
> migration/migration.c | 55 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..32f5143dfd 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -27,7 +27,6 @@
> #include "qemu/cutils.h"
>
> #ifdef WIN32
> -const char *exec_get_cmd_path(void);
> const char *exec_get_cmd_path(void)
> {
> g_autofree char *detected_path = g_new(char, MAX_PATH);
> diff --git a/migration/exec.h b/migration/exec.h
> index b210ffde7a..736cd71028 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -19,6 +19,10 @@
>
> #ifndef QEMU_MIGRATION_EXEC_H
> #define QEMU_MIGRATION_EXEC_H
> +
> +#ifdef WIN32
> +const char *exec_get_cmd_path(void);
> +#endif
> void exec_start_incoming_migration(const char *host_port, Error **errp);
>
> void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> diff --git a/migration/migration.c b/migration/migration.c
> index 6d3cf5d5cd..dcbd509d56 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -65,6 +65,7 @@
> #include "sysemu/qtest.h"
> #include "options.h"
> #include "sysemu/dirtylimit.h"
> +#include "qemu/sockets.h"
>
> static NotifierList migration_state_notifiers =
> NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -427,15 +428,64 @@ void migrate_add_address(SocketAddress *address)
> QAPI_CLONE(SocketAddress, address));
> }
>
> +static bool migrate_uri_parse(const char *uri,
> + MigrationAddress **channel,
> + Error **errp)
> +{
> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
This cannot be g_autoptr because you're passing it out of scope at the
end of the function.
> + SocketAddress *saddr = &addr->u.socket;
This attribution is useless. Down below you overwrite it with the result
of socket_parse.
> + InetSocketAddress *isock = &addr->u.rdma;
> + strList **tail = &addr->u.exec.args;
> +
> + if (strstart(uri, "exec:", NULL)) {
> + addr->transport = MIGRATION_ADDRESS_TYPE_EXEC;
> +#ifdef WIN32
> + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> + QAPI_LIST_APPEND(tail, g_strdup("/c"));
> +#else
> + QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> + QAPI_LIST_APPEND(tail, g_strdup("-c"));
> +#endif
> + QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> + } else if (strstart(uri, "rdma:", NULL)) {
> + if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
> + qapi_free_InetSocketAddress(isock);
> + return false;
> + }
> + addr->transport = MIGRATION_ADDRESS_TYPE_RDMA;
> + } else if (strstart(uri, "tcp:", NULL) ||
> + strstart(uri, "unix:", NULL) ||
> + strstart(uri, "vsock:", NULL) ||
> + strstart(uri, "fd:", NULL)) {
> + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET;
> + saddr = socket_parse(uri, errp);
> + if (!saddr) {
> + qapi_free_SocketAddress(saddr);
Shouldn't free here. socket_parse() already does so on failure.
> + return false;
> + }
Then here you can set the values you intended to set.
addr->u.socket.type = saddr->type;
addr->u.socket.u = saddr->u;
> + } else {
> + error_setg(errp, "unknown migration protocol: %s", uri);
> + return false;
> + }
> +
> + *channel = addr;
> + return true;
> +}
> +
> static void qemu_start_incoming_migration(const char *uri, Error **errp)
> {
> const char *p = NULL;
> + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
The memory is leaked here because the pointer is overwritten below. Just
set it to NULL. You can keep the g_autoptr.
>
> /* URI is not suitable for migration? */
> if (!migration_channels_and_uri_compatible(uri, errp)) {
> return;
> }
>
> + if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> + return;
> + }
> +
> qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> if (strstart(uri, "tcp:", &p) ||
> strstart(uri, "unix:", NULL) ||
> @@ -1671,12 +1721,17 @@ void qmp_migrate(const char *uri, bool has_blk, bool
> blk,
> Error *local_err = NULL;
> MigrationState *s = migrate_get_current();
> const char *p = NULL;
> + g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
Same here.
>
> /* URI is not suitable for migration? */
> if (!migration_channels_and_uri_compatible(uri, errp)) {
> return;
> }
>
> + if (!migrate_uri_parse(uri, &channel, errp)) {
> + return;
> + }
> +
> resume_requested = has_resume && resume;
> if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> resume_requested, errp)) {
