| Message ID | 20221226053329.157905-4-het.gala@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Modified 'migrate' QAPI command for migration | expand |
On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote: > From: Author Het Gala <het.gala@nutanix.com> > > Existing uri is encoded at multiple levels to extract the relevant > migration information. > > The modified QAPI design maps migration parameters into MigrateChannel > struct before, thus avoiding double-level uri encoding. > > socket_outgoing_migration() has been depricated as It's only purpose was > uri parsing. > Renamed socket_outgoing_migration_internal() as socket_outgoing_migration(). > qemu_uri_parsing() has been introduced to parse uri string (backward > compatibility) and populate the MigrateChannel struct parameters. Note that > the function will no longer be needed once the 'uri' parameter is depricated. > > Suggested-by: Daniel P. Berrange <berrange@redhat.com> > Suggested-by: Manish Mishra <manish.mishra@nutanix.com> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com> > Signed-off-by: Het Gala <het.gala@nutanix.com> > --- > migration/migration.c | 78 +++++++++++++++++++++++++++++++++++-------- > migration/socket.c | 15 +-------- > migration/socket.h | 3 +- > 3 files changed, 67 insertions(+), 29 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 1b6e62612a..36de9f6a6b 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -61,6 +61,7 @@ > #include "sysemu/cpus.h" > #include "yank_functions.h" > #include "sysemu/qtest.h" > +#include "qemu/sockets.h" > > #define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */ > > @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address) > QAPI_CLONE(SocketAddress, address)); > } > > +static void qemu_uri_parsing(const char *uri, > + MigrateChannel **channel, > + Error **errp) Coding style would prefer 'bool' instad of 'void'... Also lets call this 'migrate_uri_parse' > +{ > + Error *local_err = NULL; > + const char *p = NULL; > + MigrateChannel *val = g_new0(MigrateChannel, 1); > + MigrateAddress *addrs = g_new0(MigrateAddress, 1); > + SocketAddress *saddr = g_new0(SocketAddress, 1); > + > + if (strstart(uri, "exec:", &p)) { > + addrs->transport = MIGRATE_TRANSPORT_EXEC; > + addrs->u.exec.exec_str = g_strdup(p + strlen("exec:")); > + } else if (strstart(uri, "rdma:", NULL)) { > + addrs->transport = MIGRATE_TRANSPORT_RDMA; > + addrs->u.rdma.rdma_str = g_strdup(p + strlen("rdma:")); > + } 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.socket_type = saddr; > + } > + val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN; > + val->addr = addrs; > + *channel = val; > + > + if (local_err) { > + error_propagate(errp, local_err); ... 'return false'; > + } ... 'return true;' > +} > + > static void qemu_start_incoming_migration(const char *uri, Error **errp) > { > const char *p = NULL; > @@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk, > { > Error *local_err = NULL; > MigrationState *s = migrate_get_current(); > - const char *p = NULL; > + MigrateAddress *addrs = g_new0(MigrateAddress, 1); > + SocketAddress *saddr = g_new0(SocketAddress, 1); > > if (!migrate_prepare(s, has_blk && blk, has_inc && inc, > has_resume && resume, errp)) { > @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk, > } > } > > + /* > + * motive here is just to have checks and convert uri into > + * socketaddress struct > + */ > + if (uri && channel) { > + error_setg(errp, "uri and channels options should be" > + "mutually exclusive"); Needs a 'return' statement after reporting the error. ALso, this check should be moved to the earlier patch that introduced the 'channel' field. > + } else if (uri) { > + qemu_uri_parsing(uri, &channel, &local_err); Needs to 'return' on error, eg } else if (uri && !qemu_uri_parsing(...)) return; > + } > + With regards, Daniel
On 09/01/23 7:44 pm, Daniel P. Berrangé wrote: > On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote: >> From: Author Het Gala <het.gala@nutanix.com> >> >> Existing uri is encoded at multiple levels to extract the relevant >> migration information. >> >> The modified QAPI design maps migration parameters into MigrateChannel >> struct before, thus avoiding double-level uri encoding. >> >> socket_outgoing_migration() has been depricated as It's only purpose was >> uri parsing. >> Renamed socket_outgoing_migration_internal() as socket_outgoing_migration(). >> qemu_uri_parsing() has been introduced to parse uri string (backward >> compatibility) and populate the MigrateChannel struct parameters. Note that >> the function will no longer be needed once the 'uri' parameter is depricated. >> >> >> >> @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address) >> QAPI_CLONE(SocketAddress, address)); >> } >> >> +static void qemu_uri_parsing(const char *uri, >> + MigrateChannel **channel, >> + Error **errp) > Coding style would prefer 'bool' instad of 'void'... > > Also lets call this 'migrate_uri_parse' Sure, Ack. Will change it in the upcoming patch. >> + if (strstart(uri, "exec:", &p)) { >> + addrs->transport = MIGRATE_TRANSPORT_EXEC; >> + addrs->u.exec.exec_str = g_strdup(p + strlen("exec:")); >> + } else if (strstart(uri, "rdma:", NULL)) { >> + addrs->transport = MIGRATE_TRANSPORT_RDMA; >> + addrs->u.rdma.rdma_str = g_strdup(p + strlen("rdma:")); >> + } 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.socket_type = saddr; >> + } >> + val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN; >> + val->addr = addrs; >> + *channel = val; >> + >> + if (local_err) { >> + error_propagate(errp, local_err); > ... 'return false'; >> + } > ... 'return true;' Ack. >> +} >> + >> static void qemu_start_incoming_migration(const char *uri, Error **errp) >> { >> const char *p = NULL; >> @@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk, >> { >> Error *local_err = NULL; >> MigrationState *s = migrate_get_current(); >> - const char *p = NULL; >> + MigrateAddress *addrs = g_new0(MigrateAddress, 1); >> + SocketAddress *saddr = g_new0(SocketAddress, 1); >> >> if (!migrate_prepare(s, has_blk && blk, has_inc && inc, >> has_resume && resume, errp)) { >> @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk, >> } >> } >> >> + /* >> + * motive here is just to have checks and convert uri into >> + * socketaddress struct >> + */ >> + if (uri && channel) { >> + error_setg(errp, "uri and channels options should be" >> + "mutually exclusive"); > Needs a 'return' statement after reporting the error. ALso, this > check should be moved to the earlier patch that introduced the > 'channel' field. Okay sure. Will introduce the check in 2nd patch itself. >> + } else if (uri) { >> + qemu_uri_parsing(uri, &channel, &local_err); > Needs to 'return' on error, eg > > } else if (uri && !qemu_uri_parsing(...)) > return; Ack. > With regards, > Daniel Regards, Het Gala
diff --git a/migration/migration.c b/migration/migration.c index 1b6e62612a..36de9f6a6b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -61,6 +61,7 @@ #include "sysemu/cpus.h" #include "yank_functions.h" #include "sysemu/qtest.h" +#include "qemu/sockets.h" #define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */ @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address) QAPI_CLONE(SocketAddress, address)); } +static void qemu_uri_parsing(const char *uri, + MigrateChannel **channel, + Error **errp) +{ + Error *local_err = NULL; + const char *p = NULL; + MigrateChannel *val = g_new0(MigrateChannel, 1); + MigrateAddress *addrs = g_new0(MigrateAddress, 1); + SocketAddress *saddr = g_new0(SocketAddress, 1); + + if (strstart(uri, "exec:", &p)) { + addrs->transport = MIGRATE_TRANSPORT_EXEC; + addrs->u.exec.exec_str = g_strdup(p + strlen("exec:")); + } else if (strstart(uri, "rdma:", NULL)) { + addrs->transport = MIGRATE_TRANSPORT_RDMA; + addrs->u.rdma.rdma_str = g_strdup(p + strlen("rdma:")); + } 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.socket_type = saddr; + } + val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN; + val->addr = addrs; + *channel = val; + + if (local_err) { + error_propagate(errp, local_err); + } +} + static void qemu_start_incoming_migration(const char *uri, Error **errp) { const char *p = NULL; @@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk, { Error *local_err = NULL; MigrationState *s = migrate_get_current(); - const char *p = NULL; + MigrateAddress *addrs = g_new0(MigrateAddress, 1); + SocketAddress *saddr = g_new0(SocketAddress, 1); if (!migrate_prepare(s, has_blk && blk, has_inc && inc, has_resume && resume, errp)) { @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk, } } + /* + * motive here is just to have checks and convert uri into + * socketaddress struct + */ + if (uri && channel) { + error_setg(errp, "uri and channels options should be" + "mutually exclusive"); + } else if (uri) { + qemu_uri_parsing(uri, &channel, &local_err); + } + migrate_protocol_allow_multi_channels(false); - if (strstart(uri, "tcp:", &p) || - strstart(uri, "unix:", NULL) || - strstart(uri, "vsock:", NULL)) { - migrate_protocol_allow_multi_channels(true); - socket_start_outgoing_migration(s, p ? p : uri, &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); + 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); + } + #ifdef CONFIG_RDMA + } else if (addrs->transport == MIGRATE_TRANSPORT_RDMA) { + rdma_start_outgoing_migration(s, addrs->u.rdma.rdma_str, &local_err); + #endif + } else if (addrs->transport == MIGRATE_TRANSPORT_EXEC) { + exec_start_outgoing_migration(s, addrs->u.exec.exec_str, &local_err); } else { if (!(has_resume && resume)) { yank_unregister_instance(MIGRATION_YANK_INSTANCE); diff --git a/migration/socket.c b/migration/socket.c index e6fdf3c5e1..ecf98b7e6b 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -107,8 +107,7 @@ out: object_unref(OBJECT(sioc)); } -static void -socket_start_outgoing_migration_internal(MigrationState *s, +void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr, Error **errp) { @@ -134,18 +133,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) diff --git a/migration/socket.h b/migration/socket.h index dc54df4e6c..95c9c166ec 100644 --- a/migration/socket.h +++ b/migration/socket.h @@ -19,6 +19,7 @@ #include "io/channel.h" #include "io/task.h" +#include "io/channel-socket.h" void socket_send_channel_create(QIOTaskFunc f, void *data); QIOChannel *socket_send_channel_create_sync(Error **errp); @@ -26,6 +27,6 @@ int socket_send_channel_destroy(QIOChannel *send); void socket_start_incoming_migration(const char *str, Error **errp); -void socket_start_outgoing_migration(MigrationState *s, const char *str, +void socket_start_outgoing_migration(MigrationState *s, SocketAddress *saddr, Error **errp); #endif