On Thu, Jul 13, 2023 at 10:57:12AM +0000, Het Gala wrote:
> Integrate MigrateChannelList with all transport backends
> (socket, exec and rdma) for both src and dest migration
> endpoints.
>
> Suggested-by: Aravind Retnakaran <[email protected]>
> Signed-off-by: Het Gala <[email protected]>
> ---
> migration/migration.c | 77 ++++++++++++++++++++++++++++---------------
> migration/socket.c | 5 ++-
> 2 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 62894952ca..43c7e4b525 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -426,9 +426,10 @@ void migrate_add_address(SocketAddress *address)
> }
>
> static bool migrate_uri_parse(const char *uri,
> - MigrationAddress **channel,
> + MigrationChannel **channel,
> Error **errp)
> {
> + g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
> g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
> SocketAddress *saddr = &addr->u.socket;
> InetSocketAddress *isock = &addr->u.rdma;
> @@ -464,7 +465,9 @@ static bool migrate_uri_parse(const char *uri,
> return false;
> }
>
> - *channel = addr;
> + val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
> + val->addr = addr;
> + *channel = val;
> return true;
> }
>
> @@ -472,7 +475,8 @@ static void qemu_start_incoming_migration(const char
> *uri, bool has_channels,
> MigrationChannelList *channels,
> Error **errp)
> {
> - g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> + g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>
> /*
> * Having preliminary checks for uri and channel
> @@ -482,20 +486,29 @@ static void qemu_start_incoming_migration(const char
> *uri, bool has_channels,
> "exclusive; exactly one of the two should be present in "
> "'migrate-incoming' qmp command ");
> return;
> - }
> -
> - if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> - return;
> + } else if (channels) {
> + /* To verify that Migrate channel list has only item */
> + if (channels->next) {
> + error_setg(errp, "Channel list has more than one entries");
> + return;
> + }
> + channel = channels->value;
> + addr = channel->addr;
> + } else {
> + /* caller uses the old URI syntax */
> + if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> + return;
> + }
> }
>
> /* transport mechanism not suitable for migration? */
> - if (!migration_channels_and_transport_compatible(channel, errp)) {
> + if (!migration_channels_and_transport_compatible(addr, errp)) {
This is strange - the API contract of
migration_channels_and_transport_compatible
hasn't changed, so replacing MigrationChannel object with MigrationAddress is
surely not going to compile. Also the '} else {' branch above doesn't
set 'addr' so it will be NULL in that codepath
> return;
> }
>
> qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> - if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> - SocketAddress *saddr = &channel->u.socket;
> + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> + SocketAddress *saddr = &addr->u.socket;
> if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -504,11 +517,11 @@ static void qemu_start_incoming_migration(const char
> *uri, bool has_channels,
> fd_start_incoming_migration(saddr->u.fd.str, errp);
> }
> #ifdef CONFIG_RDMA
> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> - rdma_start_incoming_migration(&channel->u.rdma, errp);
> -#endif
> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> - exec_start_incoming_migration(channel->u.exec.args, errp);
> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> + rdma_start_incoming_migration(&addr->u.rdma, errp);
> + #endif
> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> + exec_start_incoming_migration(addr->u.exec.args, errp);
> } else {
> error_setg(errp, "unknown migration protocol: %s", uri);
> }
> @@ -1708,7 +1721,8 @@ void qmp_migrate(const char *uri, bool has_channels,
> bool resume_requested;
> Error *local_err = NULL;
> MigrationState *s = migrate_get_current();
> - g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
> + g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>
> /*
> * Having preliminary checks for uri and channel
> @@ -1718,14 +1732,23 @@ void qmp_migrate(const char *uri, bool has_channels,
> "exclusive; exactly one of the two should be present in "
> "'migrate' qmp command ");
> return;
> - }
> -
> - if (!migrate_uri_parse(uri, &channel, errp)) {
> - return;
> + } else if (channels) {
> + /* To verify that Migrate channel list has only item */
> + if (channels->next) {
> + error_setg(errp, "Channel list has more than one entries");
> + return;
> + }
> + channel = channels->value;
> + addr = channel->addr;
> + } else {
> + /* caller uses the old URI syntax */
> + if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> + return;
> + }
> }
>
> /* transport mechanism not suitable for migration? */
> - if (!migration_channels_and_transport_compatible(channel, errp)) {
> + if (!migration_channels_and_transport_compatible(addr, errp)) {
> return;
> }
>
> @@ -1742,8 +1765,8 @@ void qmp_migrate(const char *uri, bool has_channels,
> }
> }
>
> - if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> - SocketAddress *saddr = &channel->u.socket;
> + if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> + SocketAddress *saddr = &addr->u.socket;
> if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> @@ -1752,11 +1775,11 @@ void qmp_migrate(const char *uri, bool has_channels,
> fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> }
> #ifdef CONFIG_RDMA
> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> - rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> + rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
> #endif
> - } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> - exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
> + } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> + exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
> } else {
> if (!resume_requested) {
> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> diff --git a/migration/socket.c b/migration/socket.c
> index 8e7430b266..98e3ea1514 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -28,6 +28,8 @@
> #include "trace.h"
> #include "postcopy-ram.h"
> #include "options.h"
> +#include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-sockets.h"
>
> struct SocketOutgoingArgs {
> SocketAddress *saddr;
> @@ -114,12 +116,13 @@ void socket_start_outgoing_migration(MigrationState *s,
> {
> QIOChannelSocket *sioc = qio_channel_socket_new();
> struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
> + SocketAddress *addr = QAPI_CLONE(SocketAddress, saddr);
>
> data->s = s;
>
> /* in case previous migration leaked it */
> qapi_free_SocketAddress(outgoing_args.saddr);
> - outgoing_args.saddr = saddr;
> + outgoing_args.saddr = addr;
>
> if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
> data->hostname = g_strdup(saddr->u.inet.host);
This patch isn't changing the API contract of socket_start_outgoing_migration,
so if this QAPI_CLONE is indeed required, that suggests it should have been
in an earlier patch which introduced the pre-requisite to clone.
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 :|
