On 09/02/23 4:10 pm, Daniel P. Berrangé wrote:
On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
In existing senario, 'migrate' QAPI argument - string uri, is encoded
twice to extract migration parameters for stream connection. This is
not a good representation of migration wire protocol as it is a data
encoding scheme within a data encoding scheme. Qemu should be able to
directly work with results from QAPI without having to do a second
level parsing.
Modified 'migrate' QAPI design supports well defined MigrateChannel
struct which plays important role in avoiding double encoding
of uri strings.
qemu_uri_parsing() parses uri string (kept for backward
compatibility) and populate the MigrateChannel struct parameters.
Migration code flow for all required migration transport types -
socket, exec and rdma is modified.
diff --git a/migration/migration.c b/migration/migration.c
index f6dd8dbb03..accbf72a18 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -63,6 +63,7 @@
#include "sysemu/cpus.h"
#include "yank_functions.h"
#include "sysemu/qtest.h"
+#include "qemu/sockets.h"
#include "ui/qemu-spice.h"
#define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */
@@ -489,6 +490,44 @@ void migrate_add_address(SocketAddress *address)
QAPI_CLONE(SocketAddress, address));
}
+static bool migrate_uri_parse(const char *uri,
+ MigrateChannel **channel,
+ Error **errp)
+{
+ Error *local_err = NULL;
+ MigrateChannel *val = g_new0(MigrateChannel, 1);
+ MigrateAddress *addrs = g_new0(MigrateAddress, 1);
+ SocketAddress *saddr = g_new0(SocketAddress, 1);
+ InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
+
+ if (strstart(uri, "exec:", NULL)) {
+ addrs->transport = MIGRATE_TRANSPORT_EXEC;
+ addrs->u.exec.data = str_split_at_comma(uri + strlen("exec:"));
Huh, what is the purpose of using 'str_split_at_comma' ? The format
of this is "exec:<shell command>", because it is run as:
const char *argv[] = { "/bin/sh", "-c", command, NULL };
we should not be trying to parse the bit after 'exec:' at all,
and certainly not splitting it on commas which makes no sense
for a shell command.
I would have expected something more like this:
strList **tail = &addrs->u.exec.data;
QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
QAPI_LIST_APPEND(tail, g_strdup("-c"));
QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
Oh, my bad Daniel. I thought for exec as string it would represent
something like "exec:/bin/sh,-c,<shell-command>". But you are right.
Because I interpreted it wrongly, I wanted to include this function
'str_split_at_comma' and that's the reason, I had to introduce first
patch unecessarily.
Thankyou for pointing it out Daniel. I will look into it properly in the
upcoming patchset, and your solution also makes sense.
+ addrs = channel->addr;
+ saddr = channel->addr->u.socket.socket_type;
+ if (addrs->transport == MIGRATE_TRANSPORT_SOCKET) {
+ if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
+ saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
+ saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
+ migrate_protocol_allow_multi_channels(true);
+ 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);
This is probably a sign that fd_start_outgoing_migration() should
be folded into the socket_start_outgoing_migration() method, but
that's a separate cleanup from this patch.
I agree Daniel. 'fd' being a part of SocketAddress, here need to show it
explicitly makes less sense.
With regards,
Daniel
Regards,
Het Gala