Just so that, there is a wider attention, I will try to address and
discuss the comments from Daniel and Juan both here, as many of them
seems to be overlapping. I hope that is fine with the maintainers.
On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
This patch introduces code that can parse 'uri' string parameter and
spit out 'MigrateAddress' struct. All the required migration parameters
are stored in the struct.
Suggested-by: Aravind Retnakaran <aravind.retnaka...@nutanix.com>
Signed-off-by: Het Gala <het.g...@nutanix.com>
---
migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 61 insertions(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 0ee07802a5..a7e4e286aa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -64,6 +64,7 @@
#include "yank_functions.h"
#include "sysemu/qtest.h"
#include "options.h"
+#include "qemu/sockets.h"
static NotifierList migration_state_notifiers =
NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
QAPI_CLONE(SocketAddress, address));
}
+static bool migrate_uri_parse(const char *uri,
+ MigrateAddress **channel,
+ Error **errp)
+{
+ Error *local_err = NULL;
+ MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+ SocketAddress *saddr;
+ InetSocketAddress *isock = &addrs->u.rdma;
+ strList **tail = &addrs->u.exec.args;
+
+ if (strstart(uri, "exec:", NULL)) {
+ addrs->transport = MIGRATE_TRANSPORT_EXEC;
+ QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
+ QAPI_LIST_APPEND(tail, g_strdup("-c"));
+ QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
+ } else if (strstart(uri, "rdma:", NULL) &&
+ !inet_parse(isock, uri + strlen("rdma:"), errp)) {
+ addrs->transport = MIGRATE_TRANSPORT_RDMA;
I would have this as
} else if (strstart(uri, "rdma:", NULL)) {
if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
addrs->transport = MIGRATE_TRANSPORT_RDMA;
}
as IMHO it is bad practice to have control pass to the next
else if clause when inet_parse() fails, as we know this is
only an RDMA addr
Ack. I will change in the next patch.
Also you need to use '&local_err' not 'errp' in the inet_parse
call, otherwise the later code block for cleanup won't run.
Yes, thanks for pointing it out Daniel. Will modify that.
Also, Juan is of the opinion that we could omit 'local_error' variable
and try to address and free the memory there itself. For ex:
if (saddr == NULL) {
qapi_free_MigrateAddress(addrs);
return false;
}
Or, Daniel, can I also define here the variables like you suggested down
in the patch ? or is it used in some special case or I am missing
something ?
g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);
So we would not have to worry to free MigrateAddress struct.
+ } else if (strstart(uri, "tcp:", NULL) ||
+ strstart(uri, "unix:", NULL) ||
+ strstart(uri, "vsock:", NULL) ||
+ strstart(uri, "fd:", NULL)) {
+ addrs->transport = MIGRATE_TRANSPORT_SOCKET;
+ saddr = socket_parse(uri, &local_err);
+ addrs->u.socket = *saddr;
Protect with
if (saddr != NULL) {
addrs->u.socket = *saddr;
}
+ }
+
+ if (local_err) {
+ qapi_free_MigrateAddress(addrs);
+ qapi_free_SocketAddress(saddr);
+ qapi_free_InetSocketAddress(isock);
+ error_propagate(errp, local_err);
+ return false;
+ }
+
+ *channel = addrs;
+ return true;
+}
+
static void qemu_start_incoming_migration(const char *uri, Error **errp)
{
const char *p = NULL;
+ MigrateAddress *channel = g_new0(MigrateAddress, 1);
Avoid the later 'out:' cleanup block by using:
g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
Ack. I think this also solves the doubt raised by Juan "I wish, I really
wish, that there was a way to free things on error". Am I right ?
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
- return;
+ goto out;
+ }
+
+ if (uri && !migrate_uri_parse(uri, &channel, errp)) {
+ error_setg(errp, "Error parsing uri");
This error_setg() is overwriting the error migrate_uri_parse already
set, so just drop it.
Okay, I got the point. I thought error_setg() would add error statement
on top of errp, but I was incorrect.
Juan also made the same point I believe. I will remove error_setg() in
the next version of patchset series.
+ goto out;
}
qapi_event_send_migration(MIGRATION_STATUS_SETUP);
@@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri,
Error **errp)
} else {
error_setg(errp, "unknown migration protocol: %s", uri);
}
+
+out:
+ qapi_free_MigrateAddress(channel);
}
static void process_incoming_migration_bh(void *opaque)
@@ -1638,10 +1687,16 @@ 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);
Here too, use g_autoptr(MigrateAddress) and drop the 'out:' block
Ack.
/* URI is not suitable for migration? */
if (!migration_channels_and_uri_compatible(uri, errp)) {
- return;
+ goto out;
+ }
+
+ if (!migrate_uri_parse(uri, &channel, &local_err)) {
+ error_setg(errp, "Error parsing uri");
+ goto out;
}
if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
@@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
error_propagate(errp, local_err);
return;
}
+
+out:
+ qapi_free_MigrateAddress(channel);
+ return;
}
void qmp_migrate_cancel(Error **errp)
--
2.22.3
With regards,
Daniel
Regards,
Het Gala
