Socket 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 socket connection into well defined SocketAddress struct.
Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
migration/exec.c | 4 ++--
migration/exec.h | 4 ++++
migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
migration/socket.c | 34 +++++----------------------------
migration/socket.h | 7 ++++---
5 files changed, 47 insertions(+), 46 deletions(-)
diff --git a/migration/exec.c b/migration/exec.c
index 2bf882bbe1..c4a3293246 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);
@@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
}
#endif
-void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
+void exec_start_outgoing_migration(MigrationState *s, const char *command,
+ Error **errp)
{
QIOChannel *ioc;
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 a7e4e286aa..61f52d2f90 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
if (strstart(uri, "exec:", NULL)) {
addrs->transport = MIGRATE_TRANSPORT_EXEC;
+#ifdef WIN32
+ QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
+#else
QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
+#endif
QAPI_LIST_APPEND(tail, g_strdup("-c"));
QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
} else if (strstart(uri, "rdma:", NULL) &&
@@ -450,8 +454,10 @@ 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;
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
@@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
goto out;
}
+ saddr = &channel->u.socket;
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
- if (strstart(uri, "tcp:", &p) ||
- strstart(uri, "unix:", NULL) ||
- strstart(uri, "vsock:", NULL)) {
- socket_start_incoming_migration(p ? p : uri, errp);
+ if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
+ if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+ saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+ saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+ socket_start_incoming_migration(saddr, &local_err);
+ } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+ fd_start_incoming_migration(saddr->u.fd.str, &local_err);
+ }
#ifdef CONFIG_RDMA
} else if (strstart(uri, "rdma:", &p)) {
rdma_start_incoming_migration(p, errp);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_incoming_migration(p, errp);
- } else if (strstart(uri, "fd:", &p)) {
- fd_start_incoming_migration(p, errp);
} else {
error_setg(errp, "unknown migration protocol: %s", uri);
}
+ if (local_err) {
+ qapi_free_SocketAddress(saddr);
+ error_propagate(errp, local_err);
+ return;
+ }
+
out:
qapi_free_MigrateAddress(channel);
}
@@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
MigrationState *s = migrate_get_current();
const char *p = NULL;
MigrateAddress *channel = g_new0(MigrateAddress, 1);
+ SocketAddress *saddr;
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
@@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
}
}
- if (strstart(uri, "tcp:", &p) ||
- strstart(uri, "unix:", NULL) ||
- strstart(uri, "vsock:", NULL)) {
- socket_start_outgoing_migration(s, p ? p : uri, &local_err);
+ saddr = &channel->u.socket;
+ if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
+ if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+ saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+ saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+ socket_start_outgoing_migration(s, saddr, &local_err);
+ } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
+ fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
+ }
#ifdef CONFIG_RDMA
} else if (strstart(uri, "rdma:", &p)) {
rdma_start_outgoing_migration(s, p, &local_err);
#endif
} else if (strstart(uri, "exec:", &p)) {
exec_start_outgoing_migration(s, p, &local_err);
- } else if (strstart(uri, "fd:", &p)) {
- fd_start_outgoing_migration(s, p, &local_err);
} else {
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
@@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
if (!(has_resume && resume)) {
yank_unregister_instance(MIGRATION_YANK_INSTANCE);
}
+ qapi_free_SocketAddress(saddr);
migrate_fd_error(s, local_err);
error_propagate(errp, local_err);
return;
diff --git a/migration/socket.c b/migration/socket.c
index 1b6f5baefb..8e7430b266 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -108,10 +108,9 @@ out:
object_unref(OBJECT(sioc));
}
-static void
-socket_start_outgoing_migration_internal(MigrationState *s,
- SocketAddress *saddr,
- Error **errp)
+void socket_start_outgoing_migration(MigrationState *s,
+ SocketAddress *saddr,
+ Error **errp)
{
QIOChannelSocket *sioc = qio_channel_socket_new();
struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
@@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
NULL);
}
-void socket_start_outgoing_migration(MigrationState *s,
- const char *str,
- Error **errp)
-{
- Error *err = NULL;
- SocketAddress *saddr = socket_parse(str, &err);
- if (!err) {
- socket_start_outgoing_migration_internal(s, saddr, &err);
- }
- error_propagate(errp, err);
-}
-
static void socket_accept_incoming_migration(QIONetListener *listener,
QIOChannelSocket *cioc,
gpointer opaque)
@@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
object_unref(OBJECT(listener));
}
-static void
-socket_start_incoming_migration_internal(SocketAddress *saddr,
- Error **errp)
+void socket_start_incoming_migration(SocketAddress *saddr,
+ Error **errp)
{
QIONetListener *listener = qio_net_listener_new();
MigrationIncomingState *mis = migration_incoming_get_current();
@@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
}
}
-void socket_start_incoming_migration(const char *str, Error **errp)
-{
- Error *err = NULL;
- SocketAddress *saddr = socket_parse(str, &err);
- if (!err) {
- socket_start_incoming_migration_internal(saddr, &err);
- }
- qapi_free_SocketAddress(saddr);
- error_propagate(errp, err);
-}
diff --git a/migration/socket.h b/migration/socket.h
index dc54df4e6c..5e4c33b8ea 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -19,13 +19,14 @@
#include "io/channel.h"
#include "io/task.h"
+#include "qemu/sockets.h"
void socket_send_channel_create(QIOTaskFunc f, void *data);
QIOChannel *socket_send_channel_create_sync(Error **errp);
int socket_send_channel_destroy(QIOChannel *send);
-void socket_start_incoming_migration(const char *str, Error **errp);
+void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
-void socket_start_outgoing_migration(MigrationState *s, const char *str,
- Error **errp);
+void socket_start_outgoing_migration(MigrationState *s,
+ SocketAddress *saddr, Error **errp);
#endif
--
2.22.3On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> Socket 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 socket connection into well defined SocketAddress struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> migration/exec.c | 4 ++--
> migration/exec.h | 4 ++++
> migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
> migration/socket.c | 34 +++++----------------------------
> migration/socket.h | 7 ++++---
> 5 files changed, 47 insertions(+), 46 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..c4a3293246 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);
> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> }
> #endif
>
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> + Error **errp)
> {
> QIOChannel *ioc;
>
> 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 a7e4e286aa..61f52d2f90 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>
> if (strstart(uri, "exec:", NULL)) {
> addrs->transport = MIGRATE_TRANSPORT_EXEC;
> +#ifdef WIN32
> + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> +#else
> QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> +#endif
This windows portability code should have been in the previous patch
I think.
> QAPI_LIST_APPEND(tail, g_strdup("-c"));
> QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> } else if (strstart(uri, "rdma:", NULL) &&
> @@ -450,8 +454,10 @@ 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;
>
> /* URI is not suitable for migration? */
> if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
> goto out;
> }
>
> + saddr = &channel->u.socket;
Accessing u.socket before checkout transport == SOCKET is bad
practice, even though this is technically safe.
> qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> - if (strstart(uri, "tcp:", &p) ||
> - strstart(uri, "unix:", NULL) ||
> - strstart(uri, "vsock:", NULL)) {
> - socket_start_incoming_migration(p ? p : uri, errp);
> + if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
THis should have
SocketAddress *saddr = &channe->u.socket
so that 'saddr' is limited in scope to where we've validated
transport == SOCKET
> + if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> + saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> + saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> + socket_start_incoming_migration(saddr, &local_err);
> + } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> + fd_start_incoming_migration(saddr->u.fd.str, &local_err);
> + }
> #ifdef CONFIG_RDMA
> } else if (strstart(uri, "rdma:", &p)) {
> rdma_start_incoming_migration(p, errp);
> #endif
> } else if (strstart(uri, "exec:", &p)) {
> exec_start_incoming_migration(p, errp);
> - } else if (strstart(uri, "fd:", &p)) {
> - fd_start_incoming_migration(p, errp);
> } else {
> error_setg(errp, "unknown migration protocol: %s", uri);
> }
>
> + if (local_err) {
> + qapi_free_SocketAddress(saddr);
> + error_propagate(errp, local_err);
> + return;
THis leaks 'channel', and free's 'saddr' which actually belongs
to channel.
With my comments on the previous patch suggesting g_autoptr for
'channel', we don't need any free calls for 'saddr' or 'channel'.
> + }
> +
> out:
> qapi_free_MigrateAddress(channel);
> }
> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> MigrationState *s = migrate_get_current();
> const char *p = NULL;
> MigrateAddress *channel = g_new0(MigrateAddress, 1);
> + SocketAddress *saddr;
>
> /* URI is not suitable for migration? */
> if (!migration_channels_and_uri_compatible(uri, errp)) {
> @@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> }
> }
>
> - if (strstart(uri, "tcp:", &p) ||
> - strstart(uri, "unix:", NULL) ||
> - strstart(uri, "vsock:", NULL)) {
> - socket_start_outgoing_migration(s, p ? p : uri, &local_err);
> + saddr = &channel->u.socket;
Again, put this *after* checking transport == SOCKET
> + if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> + if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> + saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> + saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> + socket_start_outgoing_migration(s, saddr, &local_err);
> + } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> + fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> + }
> #ifdef CONFIG_RDMA
> } else if (strstart(uri, "rdma:", &p)) {
> rdma_start_outgoing_migration(s, p, &local_err);
> #endif
> } else if (strstart(uri, "exec:", &p)) {
> exec_start_outgoing_migration(s, p, &local_err);
> - } else if (strstart(uri, "fd:", &p)) {
> - fd_start_outgoing_migration(s, p, &local_err);
> } else {
> if (!(has_resume && resume)) {
> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> if (!(has_resume && resume)) {
> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> }
> + qapi_free_SocketAddress(saddr);
This saddr pointer belongs to 'channel' which must be freed.
> migrate_fd_error(s, local_err);
> error_propagate(errp, local_err);
> return;
> diff --git a/migration/socket.c b/migration/socket.c
> index 1b6f5baefb..8e7430b266 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -108,10 +108,9 @@ out:
> object_unref(OBJECT(sioc));
> }
>
> -static void
> -socket_start_outgoing_migration_internal(MigrationState *s,
> - SocketAddress *saddr,
> - Error **errp)
> +void socket_start_outgoing_migration(MigrationState *s,
> + SocketAddress *saddr,
> + Error **errp)
> {
> QIOChannelSocket *sioc = qio_channel_socket_new();
> struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
> @@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
> NULL);
> }
>
> -void socket_start_outgoing_migration(MigrationState *s,
> - const char *str,
> - Error **errp)
> -{
> - Error *err = NULL;
> - SocketAddress *saddr = socket_parse(str, &err);
> - if (!err) {
> - socket_start_outgoing_migration_internal(s, saddr, &err);
> - }
> - error_propagate(errp, err);
> -}
> -
> static void socket_accept_incoming_migration(QIONetListener *listener,
> QIOChannelSocket *cioc,
> gpointer opaque)
> @@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
> object_unref(OBJECT(listener));
> }
>
> -static void
> -socket_start_incoming_migration_internal(SocketAddress *saddr,
> - Error **errp)
> +void socket_start_incoming_migration(SocketAddress *saddr,
> + Error **errp)
> {
> QIONetListener *listener = qio_net_listener_new();
> MigrationIncomingState *mis = migration_incoming_get_current();
> @@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
> }
> }
>
> -void socket_start_incoming_migration(const char *str, Error **errp)
> -{
> - Error *err = NULL;
> - SocketAddress *saddr = socket_parse(str, &err);
> - if (!err) {
> - socket_start_incoming_migration_internal(saddr, &err);
> - }
> - qapi_free_SocketAddress(saddr);
> - error_propagate(errp, err);
> -}
> diff --git a/migration/socket.h b/migration/socket.h
> index dc54df4e6c..5e4c33b8ea 100644
> --- a/migration/socket.h
> +++ b/migration/socket.h
> @@ -19,13 +19,14 @@
>
> #include "io/channel.h"
> #include "io/task.h"
> +#include "qemu/sockets.h"
>
> void socket_send_channel_create(QIOTaskFunc f, void *data);
> QIOChannel *socket_send_channel_create_sync(Error **errp);
> int socket_send_channel_destroy(QIOChannel *send);
>
> -void socket_start_incoming_migration(const char *str, Error **errp);
> +void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
>
> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
> - Error **errp);
> +void socket_start_outgoing_migration(MigrationState *s,
> + SocketAddress *saddr, Error **errp);
> #endif
> --
> 2.22.3
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>> Socket 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 socket connection into well defined SocketAddress struct.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>> migration/exec.c | 4 ++--
>> migration/exec.h | 4 ++++
>> migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>> migration/socket.c | 34 +++++----------------------------
>> migration/socket.h | 7 ++++---
>> 5 files changed, 47 insertions(+), 46 deletions(-)
>>
>> diff --git a/migration/exec.c b/migration/exec.c
>> index 2bf882bbe1..c4a3293246 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)
Even this, I will shift it to the 2nd patch, where I need to move
exec_get_cmd_path() out accross other file (migration.c).
>> {
>> g_autofree char *detected_path = g_new(char, MAX_PATH);
>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>> }
>> #endif
>>
>> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
>> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
>> + Error **errp)
>> {
>> QIOChannel *ioc;
Sure, Juan. Will change this in the 2nd patch itself instead of here. I
am not very convinved why should we have a different patch all together
for this, because we are just using this code outside this file in my
opinion? But if you still think so, I can make a different patch for that.
>>
>> 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 a7e4e286aa..61f52d2f90 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
>>
>> if (strstart(uri, "exec:", NULL)) {
>> addrs->transport = MIGRATE_TRANSPORT_EXEC;
>> +#ifdef WIN32
>> + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
>> +#else
>> QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
>> +#endif
> This windows portability code should have been in the previous patch
> I think.
Ack, yes, it could have also been added in the 2nd patch. Will
accomodate windows portability change there itself.
>> QAPI_LIST_APPEND(tail, g_strdup("-c"));
>> QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
>> } else if (strstart(uri, "rdma:", NULL) &&
>> @@ -450,8 +454,10 @@ 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;
>>
>> /* URI is not suitable for migration? */
>> if (!migration_channels_and_uri_compatible(uri, errp)) {
>> @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>> goto out;
>> }
>>
>> + saddr = &channel->u.socket;
> Accessing u.socket before checkout transport == SOCKET is bad
> practice, even though this is technically safe.
Ok, Ack. I understand your point, before defining whether transport
could be a socket or not, it is not a good way representing and defining
it earlier.
Thanks for pointing it out Daniel. I will change this in all the places,
where I have accessed it before confirming if the transport == socket.
>> qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> - if (strstart(uri, "tcp:", &p) ||
>> - strstart(uri, "unix:", NULL) ||
>> - strstart(uri, "vsock:", NULL)) {
>> - socket_start_incoming_migration(p ? p : uri, errp);
>> + if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> THis should have
>
> SocketAddress *saddr = &channe->u.socket
>
> so that 'saddr' is limited in scope to where we've validated
> transport == SOCKET
Yes, Ack. Makes sense.
>> + if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> + saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> + saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> + socket_start_incoming_migration(saddr, &local_err);
>> + } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> + fd_start_incoming_migration(saddr->u.fd.str, &local_err);
>> + }
>> #ifdef CONFIG_RDMA
>> } else if (strstart(uri, "rdma:", &p)) {
>> rdma_start_incoming_migration(p, errp);
>> #endif
>> } else if (strstart(uri, "exec:", &p)) {
>> exec_start_incoming_migration(p, errp);
>> - } else if (strstart(uri, "fd:", &p)) {
>> - fd_start_incoming_migration(p, errp);
>> } else {
>> error_setg(errp, "unknown migration protocol: %s", uri);
>> }
>>
>> + if (local_err) {
>> + qapi_free_SocketAddress(saddr);
>> + error_propagate(errp, local_err);
>> + return;
Juan, I get your point. But I think, we won't be needing local_err at
all, if I use g_autoptr for 'channel' and 'saddr' is a part of
'channel'. Let me have a v2 patchset and if it is still not convinving,
we can have a discussion on this.
> THis leaks 'channel', and free's 'saddr' which actually belongs
> to channel.
>
> With my comments on the previous patch suggesting g_autoptr for
> 'channel', we don't need any free calls for 'saddr' or 'channel'.
Right. With g_autoptr used for freeing 'channel' in last patch, we wont
have to worry about freeing 'saddr' at all. Thanks Daniel
if (local_err) {
qapi_free_SocketAddress(saddr);
error_propagate(errp, local_err);
return;
}
And after changing the position for assigning 'saddr' and using
g_autoptr for 'channel' I believe we can get rid of 'local_error'
variable too and replace it with 'errp'. Please suggest if I am missing
something here. TIA!
>> + }
>> +
>> out:
>> qapi_free_MigrateAddress(channel);
>> }
>> @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>> MigrationState *s = migrate_get_current();
>> const char *p = NULL;
>> MigrateAddress *channel = g_new0(MigrateAddress, 1);
>> + SocketAddress *saddr;
>>
>> /* URI is not suitable for migration? */
>> if (!migration_channels_and_uri_compatible(uri, errp)) {
>> @@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>> }
>> }
>>
>> - if (strstart(uri, "tcp:", &p) ||
>> - strstart(uri, "unix:", NULL) ||
>> - strstart(uri, "vsock:", NULL)) {
>> - socket_start_outgoing_migration(s, p ? p : uri, &local_err);
>> + saddr = &channel->u.socket;
> Again, put this *after* checking transport == SOCKET
Ack.
>> + if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
>> + if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>> + saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>> + saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> + socket_start_outgoing_migration(s, saddr, &local_err);
>> + } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> + fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>> + }
>> #ifdef CONFIG_RDMA
>> } else if (strstart(uri, "rdma:", &p)) {
>> rdma_start_outgoing_migration(s, p, &local_err);
>> #endif
>> } else if (strstart(uri, "exec:", &p)) {
>> exec_start_outgoing_migration(s, p, &local_err);
>> - } else if (strstart(uri, "fd:", &p)) {
>> - fd_start_outgoing_migration(s, p, &local_err);
>> } else {
>> if (!(has_resume && resume)) {
>> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>> if (!(has_resume && resume)) {
>> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> }
>> + qapi_free_SocketAddress(saddr);
> This saddr pointer belongs to 'channel' which must be freed.
Again here, with your comment in last patch to use g_autoptr, we need
not to explicitly free 'channel' or 'saddr' right.
>> migrate_fd_error(s, local_err);
>> error_propagate(errp, local_err);
>> return;
>> diff --git a/migration/socket.c b/migration/socket.c
>> index 1b6f5baefb..8e7430b266 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -108,10 +108,9 @@ out:
>> object_unref(OBJECT(sioc));
>> }
>>
>> -static void
>> -socket_start_outgoing_migration_internal(MigrationState *s,
>> - SocketAddress *saddr,
>> - Error **errp)
>> +void socket_start_outgoing_migration(MigrationState *s,
>> + SocketAddress *saddr,
>> + Error **errp)
>> {
>> QIOChannelSocket *sioc = qio_channel_socket_new();
>> struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
>> @@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
>> NULL);
>> }
>>
>> -void socket_start_outgoing_migration(MigrationState *s,
>> - const char *str,
>> - Error **errp)
>> -{
>> - Error *err = NULL;
>> - SocketAddress *saddr = socket_parse(str, &err);
>> - if (!err) {
>> - socket_start_outgoing_migration_internal(s, saddr, &err);
>> - }
>> - error_propagate(errp, err);
>> -}
>> -
Actually Juan. I don't need this function at all, because parsing of uri
into socketAddress using socket_parse is already done. So there is no
use of having this function in the first place, so I decided to delete
this fucntion all together. Same with incoming function.
>> static void socket_accept_incoming_migration(QIONetListener *listener,
>> QIOChannelSocket *cioc,
>> gpointer opaque)
>> @@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
>> object_unref(OBJECT(listener));
>> }
>>
>> -static void
>> -socket_start_incoming_migration_internal(SocketAddress *saddr,
>> - Error **errp)
>> +void socket_start_incoming_migration(SocketAddress *saddr,
>> + Error **errp)
>> {
>> QIONetListener *listener = qio_net_listener_new();
>> MigrationIncomingState *mis = migration_incoming_get_current();
>> @@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress *saddr,
>> }
>> }
>>
>> -void socket_start_incoming_migration(const char *str, Error **errp)
>> -{
>> - Error *err = NULL;
>> - SocketAddress *saddr = socket_parse(str, &err);
>> - if (!err) {
>> - socket_start_incoming_migration_internal(saddr, &err);
>> - }
>> - qapi_free_SocketAddress(saddr);
>> - error_propagate(errp, err);
>> -}
>> diff --git a/migration/socket.h b/migration/socket.h
>> index dc54df4e6c..5e4c33b8ea 100644
>> --- a/migration/socket.h
>> +++ b/migration/socket.h
>> @@ -19,13 +19,14 @@
>>
>> #include "io/channel.h"
>> #include "io/task.h"
>> +#include "qemu/sockets.h"
>>
>> void socket_send_channel_create(QIOTaskFunc f, void *data);
>> QIOChannel *socket_send_channel_create_sync(Error **errp);
>> int socket_send_channel_destroy(QIOChannel *send);
>>
>> -void socket_start_incoming_migration(const char *str, Error **errp);
>> +void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
>>
>> -void socket_start_outgoing_migration(MigrationState *s, const char *str,
>> - Error **errp);
>> +void socket_start_outgoing_migration(MigrationState *s,
>> + SocketAddress *saddr, Error **errp);
>> #endif
>> --
>> 2.22.3
>>
> With regards,
> Daniel
Het Gala <het.gala@nutanix.com> wrote:
> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>> Socket 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 socket connection into well defined SocketAddress struct.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>> migration/exec.c | 4 ++--
>>> migration/exec.h | 4 ++++
>>> migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>>> migration/socket.c | 34 +++++----------------------------
>>> migration/socket.h | 7 ++++---
>>> 5 files changed, 47 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/migration/exec.c b/migration/exec.c
>>> index 2bf882bbe1..c4a3293246 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)
> Even this, I will shift it to the 2nd patch, where I need to move
> exec_get_cmd_path() out accross other file (migration.c).
great.
>>> {
>>> g_autofree char *detected_path = g_new(char, MAX_PATH);
>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>> }
>>> #endif
>>> -void exec_start_outgoing_migration(MigrationState *s, const char
>>> *command, Error **errp)
>>> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
>>> + Error **errp)
>>> {
>>> QIOChannel *ioc;
> Sure, Juan. Will change this in the 2nd patch itself instead of
> here. I am not very convinved why should we have a different patch all
> together for this, because we are just using this code outside this
> file in my opinion? But if you still think so, I can make a different
> patch for that.
It is up to you.
> Juan, I get your point. But I think, we won't be needing local_err at
> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> 'channel'. Let me have a v2 patchset and if it is still not
> convinving, we can have a discussion on this.
>> THis leaks 'channel', and free's 'saddr' which actually belongs
>> to channel.
>>
>> With my comments on the previous patch suggesting g_autoptr for
>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>
> Right. With g_autoptr used for freeing 'channel' in last patch, we
> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>
> if (local_err) {
> qapi_free_SocketAddress(saddr);
> error_propagate(errp, local_err);
> return;
> }
> And after changing the position for assigning 'saddr' and using
> g_autoptr for 'channel' I believe we can get rid of 'local_error'
> variable too and replace it with 'errp'. Please suggest if I am
> missing something here. TIA!
great. That is much better.
>>> -void socket_start_outgoing_migration(MigrationState *s,
>>> - const char *str,
>>> - Error **errp)
>>> -{
>>> - Error *err = NULL;
>>> - SocketAddress *saddr = socket_parse(str, &err);
>>> - if (!err) {
>>> - socket_start_outgoing_migration_internal(s, saddr, &err);
>>> - }
>>> - error_propagate(errp, err);
>>> -}
>>> -
> Actually Juan. I don't need this function at all, because parsing of
> uri into socketAddress using socket_parse is already done. So there is
> no use of having this function in the first place, so I decided to
> delete this fucntion all together. Same with incoming function.
What I mean is that this code was already there. And it was doing it
wrong. The parts that I corrected you were using this pattern, chcking
that err was NULL, intsead of cheking that saddr is NULL.
Later, Juan.
On 15/05/23 8:16 pm, Juan Quintela wrote:
> Het Gala <het.gala@nutanix.com> wrote:
>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>>> Socket 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 socket connection into well defined SocketAddress struct.
>>>>
>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>>> ---
>>>> migration/exec.c | 4 ++--
>>>> migration/exec.h | 4 ++++
>>>> migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
>>>> migration/socket.c | 34 +++++----------------------------
>>>> migration/socket.h | 7 ++++---
>>>> 5 files changed, 47 insertions(+), 46 deletions(-)
>>>>
>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>> index 2bf882bbe1..c4a3293246 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)
>> Even this, I will shift it to the 2nd patch, where I need to move
>> exec_get_cmd_path() out accross other file (migration.c).
> great.
>
>>>> {
>>>> g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>>> }
>>>> #endif
>>>> -void exec_start_outgoing_migration(MigrationState *s, const char
>>>> *command, Error **errp)
>>>> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
>>>> + Error **errp)
>>>> {
>>>> QIOChannel *ioc;
>> Sure, Juan. Will change this in the 2nd patch itself instead of
>> here. I am not very convinved why should we have a different patch all
>> together for this, because we are just using this code outside this
>> file in my opinion? But if you still think so, I can make a different
>> patch for that.
> It is up to you.
For now, I will keep it with 2nd patch. If any other maintainer also
thinks that it should be a separate patch all together of windows
support for exec, I will make a new patch for that. But thankyou for
your suggestion 😁
>> Juan, I get your point. But I think, we won't be needing local_err at
>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>> 'channel'. Let me have a v2 patchset and if it is still not
>> convinving, we can have a discussion on this.
>>> THis leaks 'channel', and free's 'saddr' which actually belongs
>>> to channel.
>>>
>>> With my comments on the previous patch suggesting g_autoptr for
>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>
>> if (local_err) {
>> qapi_free_SocketAddress(saddr);
>> error_propagate(errp, local_err);
>> return;
>> }
>> And after changing the position for assigning 'saddr' and using
>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>> variable too and replace it with 'errp'. Please suggest if I am
>> missing something here. TIA!
> great. That is much better.
Ack.
>>>> -void socket_start_outgoing_migration(MigrationState *s,
>>>> - const char *str,
>>>> - Error **errp)
>>>> -{
>>>> - Error *err = NULL;
>>>> - SocketAddress *saddr = socket_parse(str, &err);
>>>> - if (!err) {
>>>> - socket_start_outgoing_migration_internal(s, saddr, &err);
>>>> - }
>>>> - error_propagate(errp, err);
>>>> -}
>>>> -
>> Actually Juan. I don't need this function at all, because parsing of
>> uri into socketAddress using socket_parse is already done. So there is
>> no use of having this function in the first place, so I decided to
>> delete this fucntion all together. Same with incoming function.
> What I mean is that this code was already there. And it was doing it
> wrong. The parts that I corrected you were using this pattern, chcking
> that err was NULL, intsead of cheking that saddr is NULL.
Yes, I get your point. But that function is useless if socket_parse()
function is not needed. So have omitted the function all together as
socket parsing is already done in earlier patches.
> Later, Juan.
Regards,
Het Gala
On 15/05/23 8:46 pm, Het Gala wrote:
>
> On 15/05/23 8:16 pm, Juan Quintela wrote:
>> Het Gala <het.gala@nutanix.com> wrote:
>>> On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
>>>> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
>>>>> Socket 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 socket connection into well defined SocketAddress
>>>>> struct.
>>>>>
>>>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>>>> ---
>>>>> migration/exec.c | 4 ++--
>>>>> migration/exec.h | 4 ++++
>>>>> migration/migration.c | 44
>>>>> +++++++++++++++++++++++++++++++------------
>>>>> migration/socket.c | 34 +++++----------------------------
>>>>> migration/socket.h | 7 ++++---
>>>>> 5 files changed, 47 insertions(+), 46 deletions(-)
>>>>>
>>>>> diff --git a/migration/exec.c b/migration/exec.c
>>>>> index 2bf882bbe1..c4a3293246 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)
Maintainers please advice. If I want to see thatthe build is proper, how
to enable WIN32 support on a CentOS guest operating system (what all
dependencies to install, what to configure for a build which supports
WIN32) ? Any pointers ?
>>> Even this, I will shift it to the 2nd patch, where I need to move
>>> exec_get_cmd_path() out accross other file (migration.c).
>> great.
>>
>>>>> {
>>>>> g_autofree char *detected_path = g_new(char, MAX_PATH);
>>>>> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
>>>>> }
>>>>> #endif
>>>>> -void exec_start_outgoing_migration(MigrationState *s, const char
>>>>> *command, Error **errp)
>>>>> +void exec_start_outgoing_migration(MigrationState *s, const char
>>>>> *command,
>>>>> + Error **errp)
>>>>> {
>>>>> QIOChannel *ioc;
>>> Sure, Juan. Will change this in the 2nd patch itself instead of
>>> here. I am not very convinved why should we have a different patch all
>>> together for this, because we are just using this code outside this
>>> file in my opinion? But if you still think so, I can make a different
>>> patch for that.
>> It is up to you.
>
> For now, I will keep it with 2nd patch. If any other maintainer also
> thinks that it should be a separate patch all together of windows
> support for exec, I will make a new patch for that. But thankyou for
> your suggestion 😁
>
>>> Juan, I get your point. But I think, we won't be needing local_err at
>>> all, if I use g_autoptr for 'channel' and 'saddr' is a part of
>>> 'channel'. Let me have a v2 patchset and if it is still not
>>> convinving, we can have a discussion on this.
>>>> THis leaks 'channel', and free's 'saddr' which actually belongs
>>>> to channel.
>>>>
>>>> With my comments on the previous patch suggesting g_autoptr for
>>>> 'channel', we don't need any free calls for 'saddr' or 'channel'.
>>> Right. With g_autoptr used for freeing 'channel' in last patch, we
>>> wont have to worry about freeing 'saddr' at all. Thanks Daniel
>>>
>>> if (local_err) {
>>> qapi_free_SocketAddress(saddr);
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>> And after changing the position for assigning 'saddr' and using
>>> g_autoptr for 'channel' I believe we can get rid of 'local_error'
>>> variable too and replace it with 'errp'. Please suggest if I am
>>> missing something here. TIA!
>> great. That is much better.
> Ack.
>>>>> -void socket_start_outgoing_migration(MigrationState *s,
>>>>> - const char *str,
>>>>> - Error **errp)
>>>>> -{
>>>>> - Error *err = NULL;
>>>>> - SocketAddress *saddr = socket_parse(str, &err);
>>>>> - if (!err) {
>>>>> - socket_start_outgoing_migration_internal(s, saddr, &err);
>>>>> - }
>>>>> - error_propagate(errp, err);
>>>>> -}
>>>>> -
>>> Actually Juan. I don't need this function at all, because parsing of
>>> uri into socketAddress using socket_parse is already done. So there is
>>> no use of having this function in the first place, so I decided to
>>> delete this fucntion all together. Same with incoming function.
>> What I mean is that this code was already there. And it was doing it
>> wrong. The parts that I corrected you were using this pattern, chcking
>> that err was NULL, intsead of cheking that saddr is NULL.
> Yes, I get your point. But that function is useless if socket_parse()
> function is not needed. So have omitted the function all together as
> socket parsing is already done in earlier patches.
>> Later, Juan.
> Regards,
> Het Gala
On Mon, May 15, 2023 at 09:58:37PM +0530, Het Gala wrote:
>
> On 15/05/23 8:46 pm, Het Gala wrote:
> >
> > On 15/05/23 8:16 pm, Juan Quintela wrote:
> > > Het Gala <het.gala@nutanix.com> wrote:
> > > > On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> > > > > On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> > > > > > Socket 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 socket connection into well defined
> > > > > > SocketAddress struct.
> > > > > >
> > > > > > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > > > > > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > > > > > ---
> > > > > > migration/exec.c | 4 ++--
> > > > > > migration/exec.h | 4 ++++
> > > > > > migration/migration.c | 44
> > > > > > +++++++++++++++++++++++++++++++------------
> > > > > > migration/socket.c | 34 +++++----------------------------
> > > > > > migration/socket.h | 7 ++++---
> > > > > > 5 files changed, 47 insertions(+), 46 deletions(-)
> > > > > >
> > > > > > diff --git a/migration/exec.c b/migration/exec.c
> > > > > > index 2bf882bbe1..c4a3293246 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)
> Maintainers please advice. If I want to see thatthe build is proper, how to
> enable WIN32 support on a CentOS guest operating system (what all
> dependencies to install, what to configure for a build which supports WIN32)
> ? Any pointers ?
QEMU does Windows build testing using Fedora, which ships mingw
packages. If you only have centos, then use docker/podman to
get a Fedora environment in a container locally.
Alternatively push to gitlab and run a CI pipeline (see docs/devel/ci.rst
for more info)
> > > > Even this, I will shift it to the 2nd patch, where I need to move
> > > > exec_get_cmd_path() out accross other file (migration.c).
> > > great.
> > >
> > > > > > {
> > > > > > g_autofree char *detected_path = g_new(char, MAX_PATH);
> > > > > > @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> > > > > > }
> > > > > > #endif
> > > > > > -void exec_start_outgoing_migration(MigrationState *s, const char
> > > > > > *command, Error **errp)
> > > > > > +void exec_start_outgoing_migration(MigrationState *s,
> > > > > > const char *command,
> > > > > > + Error **errp)
> > > > > > {
> > > > > > QIOChannel *ioc;
> > > > Sure, Juan. Will change this in the 2nd patch itself instead of
> > > > here. I am not very convinved why should we have a different patch all
> > > > together for this, because we are just using this code outside this
> > > > file in my opinion? But if you still think so, I can make a different
> > > > patch for that.
> > > It is up to you.
> >
> > For now, I will keep it with 2nd patch. If any other maintainer also
> > thinks that it should be a separate patch all together of windows
> > support for exec, I will make a new patch for that. But thankyou for
> > your suggestion 😁
> >
> > > > Juan, I get your point. But I think, we won't be needing local_err at
> > > > all, if I use g_autoptr for 'channel' and 'saddr' is a part of
> > > > 'channel'. Let me have a v2 patchset and if it is still not
> > > > convinving, we can have a discussion on this.
> > > > > THis leaks 'channel', and free's 'saddr' which actually belongs
> > > > > to channel.
> > > > >
> > > > > With my comments on the previous patch suggesting g_autoptr for
> > > > > 'channel', we don't need any free calls for 'saddr' or 'channel'.
> > > > Right. With g_autoptr used for freeing 'channel' in last patch, we
> > > > wont have to worry about freeing 'saddr' at all. Thanks Daniel
> > > >
> > > > if (local_err) {
> > > > qapi_free_SocketAddress(saddr);
> > > > error_propagate(errp, local_err);
> > > > return;
> > > > }
> > > > And after changing the position for assigning 'saddr' and using
> > > > g_autoptr for 'channel' I believe we can get rid of 'local_error'
> > > > variable too and replace it with 'errp'. Please suggest if I am
> > > > missing something here. TIA!
> > > great. That is much better.
> > Ack.
> > > > > > -void socket_start_outgoing_migration(MigrationState *s,
> > > > > > - const char *str,
> > > > > > - Error **errp)
> > > > > > -{
> > > > > > - Error *err = NULL;
> > > > > > - SocketAddress *saddr = socket_parse(str, &err);
> > > > > > - if (!err) {
> > > > > > - socket_start_outgoing_migration_internal(s, saddr, &err);
> > > > > > - }
> > > > > > - error_propagate(errp, err);
> > > > > > -}
> > > > > > -
> > > > Actually Juan. I don't need this function at all, because parsing of
> > > > uri into socketAddress using socket_parse is already done. So there is
> > > > no use of having this function in the first place, so I decided to
> > > > delete this fucntion all together. Same with incoming function.
> > > What I mean is that this code was already there. And it was doing it
> > > wrong. The parts that I corrected you were using this pattern, chcking
> > > that err was NULL, intsead of cheking that saddr is NULL.
> > Yes, I get your point. But that function is useless if socket_parse()
> > function is not needed. So have omitted the function all together as
> > socket parsing is already done in earlier patches.
> > > Later, Juan.
> > Regards,
> > Het Gala
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Het Gala <het.gala@nutanix.com> wrote:
> Socket 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 socket connection into well defined SocketAddress struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
> migration/exec.c | 4 ++--
> migration/exec.h | 4 ++++
> migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
> migration/socket.c | 34 +++++----------------------------
> migration/socket.h | 7 ++++---
> 5 files changed, 47 insertions(+), 46 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 2bf882bbe1..c4a3293246 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);
Make this and related chunks it its own patch.
> @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> }
> #endif
>
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> + Error **errp)
> {
> QIOChannel *ioc;
>
Drop this bit. If you want to cleanup longer that 80 chars lines, do it
in a sperate patch.
> + if (local_err) {
> + qapi_free_SocketAddress(saddr);
> + error_propagate(errp, local_err);
> + return;
> + }
Aha, I see what you want to do here, but I
From include/qapi/error.h
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
*
I think that what you have to do here is drop local_err and just change
socket_start_incoming_migration() and fd_start_incoming_migration() to
return a bool.
> -void socket_start_outgoing_migration(MigrationState *s,
> - const char *str,
> - Error **errp)
> -{
> - Error *err = NULL;
> - SocketAddress *saddr = socket_parse(str, &err);
> - if (!err) {
> - socket_start_outgoing_migration_internal(s, saddr, &err);
> - }
> - error_propagate(errp, err);
> -}
And here we are. This function got it wrong, and you copied from here.
My understanding is that this function should have been written as:
void socket_start_outgoing_migration(MigrationState *s,
const char *str,
Error **errp)
{
SocketAddress *saddr = socket_parse(str, &err);
if (!saddr) {
socket_start_outgoing_migration_internal(s, saddr, &err);
}
}
> -void socket_start_incoming_migration(const char *str, Error **errp)
> -{
> - Error *err = NULL;
> - SocketAddress *saddr = socket_parse(str, &err);
> - if (!err) {
> - socket_start_incoming_migration_internal(saddr, &err);
> - }
> - qapi_free_SocketAddress(saddr);
> - error_propagate(errp, err);
> -}
Exactly the same here.
Not your fault, but can you do the changes, please?
Later, Juan.
© 2016 - 2026 Red Hat, Inc.