Message ID | 20180425112723.1111-6-quintela@redhat.com |
---|---|
State | New |
Headers | show |
Series | Multifd | expand |
On Wed, Apr 25, 2018 at 01:27:07PM +0200, Juan Quintela wrote: > Signed-off-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > --- > migration/socket.c | 28 +++++++++++++++++++++++++++- > migration/socket.h | 7 +++++++ > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/migration/socket.c b/migration/socket.c > index edf33c70cf..893a04f4cc 100644 > --- a/migration/socket.c > +++ b/migration/socket.c > @@ -29,6 +29,28 @@ > #include "trace.h" > > > +struct SocketOutgoingArgs { > + SocketAddress *saddr; > +} outgoing_args; I am not sure whether I have asked before, but... could we put this into MigrateState*? The thing is that introducing more global variables will make things scattered, and we do stuff to merge them (like the RAMState cleanup work). IMHO it saves time if we can do it from the very beginning. > + > +void socket_send_channel_create(QIOTaskFunc f, void *data) > +{ > + QIOChannelSocket *sioc = qio_channel_socket_new(); > + qio_channel_socket_connect_async(sioc, outgoing_args.saddr, > + f, data, NULL, NULL); > +} > + > +int socket_send_channel_destroy(QIOChannel *send) > +{ > + /* Remove channel */ > + object_unref(OBJECT(send)); > + if (outgoing_args.saddr) { > + qapi_free_SocketAddress(outgoing_args.saddr); > + outgoing_args.saddr = NULL; > + } > + return 0; > +} Here I would possibly avoid bothering introducing the two new APIs since AFAIU they didn't do much things, and both of them are only called once... And IMHO when we call socket_send_channel_create() in multifd_save_setup() we can initialize MultiFDSendParams->c already with the object returned by qio_channel_socket_new() if without the API, instead of waiting until multifd_new_send_channel_async() is called. Thanks,
Peter Xu <peterx@redhat.com> wrote: > On Wed, Apr 25, 2018 at 01:27:07PM +0200, Juan Quintela wrote: >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >> --- >> migration/socket.c | 28 +++++++++++++++++++++++++++- >> migration/socket.h | 7 +++++++ >> 2 files changed, 34 insertions(+), 1 deletion(-) >> >> diff --git a/migration/socket.c b/migration/socket.c >> index edf33c70cf..893a04f4cc 100644 >> --- a/migration/socket.c >> +++ b/migration/socket.c >> @@ -29,6 +29,28 @@ >> #include "trace.h" >> >> >> +struct SocketOutgoingArgs { >> + SocketAddress *saddr; >> +} outgoing_args; > > I am not sure whether I have asked before, but... could we put this > into MigrateState*? The thing is that introducing more global > variables will make things scattered, and we do stuff to merge them > (like the RAMState cleanup work). IMHO it saves time if we can do it > from the very beginning. we could, but this file don't depend at all on migration, so I didn't want to put that outside of this file, that is th ereason that it is this way. > >> + >> +void socket_send_channel_create(QIOTaskFunc f, void *data) >> +{ >> + QIOChannelSocket *sioc = qio_channel_socket_new(); >> + qio_channel_socket_connect_async(sioc, outgoing_args.saddr, >> + f, data, NULL, NULL); >> +} >> + >> +int socket_send_channel_destroy(QIOChannel *send) >> +{ >> + /* Remove channel */ >> + object_unref(OBJECT(send)); >> + if (outgoing_args.saddr) { >> + qapi_free_SocketAddress(outgoing_args.saddr); >> + outgoing_args.saddr = NULL; >> + } >> + return 0; >> +} > > Here I would possibly avoid bothering introducing the two new APIs > since AFAIU they didn't do much things, and both of them are only > called once... And IMHO when we call socket_send_channel_create() in > multifd_save_setup() we can initialize MultiFDSendParams->c already > with the object returned by qio_channel_socket_new() if without the > API, instead of waiting until multifd_new_send_channel_async() is > called. We can do it that way, but then we need the migration code to learn more about this channels stuff. You can't have both. My understanding is that the other functions are alrady quite complicated. Later, Juan.
diff --git a/migration/socket.c b/migration/socket.c index edf33c70cf..893a04f4cc 100644 --- a/migration/socket.c +++ b/migration/socket.c @@ -29,6 +29,28 @@ #include "trace.h" +struct SocketOutgoingArgs { + SocketAddress *saddr; +} outgoing_args; + +void socket_send_channel_create(QIOTaskFunc f, void *data) +{ + QIOChannelSocket *sioc = qio_channel_socket_new(); + qio_channel_socket_connect_async(sioc, outgoing_args.saddr, + f, data, NULL, NULL); +} + +int socket_send_channel_destroy(QIOChannel *send) +{ + /* Remove channel */ + object_unref(OBJECT(send)); + if (outgoing_args.saddr) { + qapi_free_SocketAddress(outgoing_args.saddr); + outgoing_args.saddr = NULL; + } + return 0; +} + static SocketAddress *tcp_build_address(const char *host_port, Error **errp) { SocketAddress *saddr; @@ -96,6 +118,11 @@ static void socket_start_outgoing_migration(MigrationState *s, struct SocketConnectData *data = g_new0(struct SocketConnectData, 1); data->s = s; + + /* in case previous migration leaked it */ + qapi_free_SocketAddress(outgoing_args.saddr); + outgoing_args.saddr = saddr; + if (saddr->type == SOCKET_ADDRESS_TYPE_INET) { data->hostname = g_strdup(saddr->u.inet.host); } @@ -107,7 +134,6 @@ static void socket_start_outgoing_migration(MigrationState *s, data, socket_connect_data_free, NULL); - qapi_free_SocketAddress(saddr); } void tcp_start_outgoing_migration(MigrationState *s, diff --git a/migration/socket.h b/migration/socket.h index 6b91e9db38..528c3b0202 100644 --- a/migration/socket.h +++ b/migration/socket.h @@ -16,6 +16,13 @@ #ifndef QEMU_MIGRATION_SOCKET_H #define QEMU_MIGRATION_SOCKET_H + +#include "io/channel.h" +#include "io/task.h" + +void socket_send_channel_create(QIOTaskFunc f, void *data); +int socket_send_channel_destroy(QIOChannel *send); + void tcp_start_incoming_migration(const char *host_port, Error **errp); void tcp_start_outgoing_migration(MigrationState *s, const char *host_port,