| Message ID | 20230512143240.192504-4-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Modified 'migrate' and 'migrate-incoming' QAPI commands for migration | expand |
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.
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) > { > 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
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
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
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
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
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(-)