| Message ID | 20240208035126.370620-3-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: cleanup TLS channel referencing | expand |
peterx@redhat.com writes: > From: Peter Xu <peterx@redhat.com> > > With a clear definition of p->c protocol, where we only set it up if the > channel is fully established (TLS or non-TLS), registered_yank boolean will > have equal meaning of "p->c != NULL". > > Drop registered_yank by checking p->c instead. > > Reviewed-by: Fabiano Rosas <farosas@suse.de> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/multifd.h | 2 -- > migration/multifd.c | 7 +++---- > 2 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/migration/multifd.h b/migration/multifd.h > index 8a1cad0996..b3fe27ae93 100644 > --- a/migration/multifd.h > +++ b/migration/multifd.h > @@ -78,8 +78,6 @@ typedef struct { > bool tls_thread_created; > /* communication channel */ > QIOChannel *c; > - /* is the yank function registered */ > - bool registered_yank; > /* packet allocated len */ > uint32_t packet_len; > /* guest page size */ > diff --git a/migration/multifd.c b/migration/multifd.c > index 4a85a6b7b3..278453cf84 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send) > > static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > { > - if (p->registered_yank) { > + if (p->c) { > migration_ioc_unregister_yank(p->c); > + multifd_send_channel_destroy(p->c); At socket_send_channel_destroy the clean up of outgoing_args.saddr will now be skipped. The failure at multifd_new_send_channel_async might have been due to TLS, in which case all of plain socket setup will have happened properly. > + p->c = NULL; > } > - multifd_send_channel_destroy(p->c); > - p->c = NULL; > qemu_sem_destroy(&p->sem); > qemu_sem_destroy(&p->sem_sync); > g_free(p->name); > @@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p, > qio_channel_set_delay(ioc, false); > > migration_ioc_register_yank(ioc); > - p->registered_yank = true; > /* Setup p->c only if the channel is completely setup */ > p->c = ioc;
On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote: > peterx@redhat.com writes: > > > From: Peter Xu <peterx@redhat.com> > > > > With a clear definition of p->c protocol, where we only set it up if the > > channel is fully established (TLS or non-TLS), registered_yank boolean will > > have equal meaning of "p->c != NULL". > > > > Drop registered_yank by checking p->c instead. > > > > Reviewed-by: Fabiano Rosas <farosas@suse.de> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/multifd.h | 2 -- > > migration/multifd.c | 7 +++---- > > 2 files changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/migration/multifd.h b/migration/multifd.h > > index 8a1cad0996..b3fe27ae93 100644 > > --- a/migration/multifd.h > > +++ b/migration/multifd.h > > @@ -78,8 +78,6 @@ typedef struct { > > bool tls_thread_created; > > /* communication channel */ > > QIOChannel *c; > > - /* is the yank function registered */ > > - bool registered_yank; > > /* packet allocated len */ > > uint32_t packet_len; > > /* guest page size */ > > diff --git a/migration/multifd.c b/migration/multifd.c > > index 4a85a6b7b3..278453cf84 100644 > > --- a/migration/multifd.c > > +++ b/migration/multifd.c > > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send) > > > > static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) > > { > > - if (p->registered_yank) { > > + if (p->c) { > > migration_ioc_unregister_yank(p->c); > > + multifd_send_channel_destroy(p->c); > > At socket_send_channel_destroy the clean up of outgoing_args.saddr will > now be skipped. The failure at multifd_new_send_channel_async might have > been due to TLS, in which case all of plain socket setup will have > happened properly. Right, IMHO it's a hack to free globals in a per-channel helper. We should have moved: if (outgoing_args.saddr) { qapi_free_SocketAddress(outgoing_args.saddr); outgoing_args.saddr = NULL; } Outside irrelevant of that.. That could be done later I guess, because we have one more guard: socket_start_outgoing_migration(): /* in case previous migration leaked it */ qapi_free_SocketAddress(outgoing_args.saddr); outgoing_args.saddr = addr; If you think proper, I can add one more patch to do that cleanup, IOW, move above free() into multifd_send_cleanup_state(). Thanks,
Peter Xu <peterx@redhat.com> writes: > On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote: >> peterx@redhat.com writes: >> >> > From: Peter Xu <peterx@redhat.com> >> > >> > With a clear definition of p->c protocol, where we only set it up if the >> > channel is fully established (TLS or non-TLS), registered_yank boolean will >> > have equal meaning of "p->c != NULL". >> > >> > Drop registered_yank by checking p->c instead. >> > >> > Reviewed-by: Fabiano Rosas <farosas@suse.de> >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > migration/multifd.h | 2 -- >> > migration/multifd.c | 7 +++---- >> > 2 files changed, 3 insertions(+), 6 deletions(-) >> > >> > diff --git a/migration/multifd.h b/migration/multifd.h >> > index 8a1cad0996..b3fe27ae93 100644 >> > --- a/migration/multifd.h >> > +++ b/migration/multifd.h >> > @@ -78,8 +78,6 @@ typedef struct { >> > bool tls_thread_created; >> > /* communication channel */ >> > QIOChannel *c; >> > - /* is the yank function registered */ >> > - bool registered_yank; >> > /* packet allocated len */ >> > uint32_t packet_len; >> > /* guest page size */ >> > diff --git a/migration/multifd.c b/migration/multifd.c >> > index 4a85a6b7b3..278453cf84 100644 >> > --- a/migration/multifd.c >> > +++ b/migration/multifd.c >> > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send) >> > >> > static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) >> > { >> > - if (p->registered_yank) { >> > + if (p->c) { >> > migration_ioc_unregister_yank(p->c); >> > + multifd_send_channel_destroy(p->c); >> >> At socket_send_channel_destroy the clean up of outgoing_args.saddr will >> now be skipped. The failure at multifd_new_send_channel_async might have >> been due to TLS, in which case all of plain socket setup will have >> happened properly. > > Right, IMHO it's a hack to free globals in a per-channel helper. We should > have moved: > > if (outgoing_args.saddr) { > qapi_free_SocketAddress(outgoing_args.saddr); > outgoing_args.saddr = NULL; > } > > Outside irrelevant of that.. > > That could be done later I guess, because we have one more guard: > > socket_start_outgoing_migration(): > /* in case previous migration leaked it */ > qapi_free_SocketAddress(outgoing_args.saddr); > outgoing_args.saddr = addr; > > If you think proper, I can add one more patch to do that cleanup, IOW, move > above free() into multifd_send_cleanup_state(). Sounds good. > > Thanks,
diff --git a/migration/multifd.h b/migration/multifd.h index 8a1cad0996..b3fe27ae93 100644 --- a/migration/multifd.h +++ b/migration/multifd.h @@ -78,8 +78,6 @@ typedef struct { bool tls_thread_created; /* communication channel */ QIOChannel *c; - /* is the yank function registered */ - bool registered_yank; /* packet allocated len */ uint32_t packet_len; /* guest page size */ diff --git a/migration/multifd.c b/migration/multifd.c index 4a85a6b7b3..278453cf84 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send) static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp) { - if (p->registered_yank) { + if (p->c) { migration_ioc_unregister_yank(p->c); + multifd_send_channel_destroy(p->c); + p->c = NULL; } - multifd_send_channel_destroy(p->c); - p->c = NULL; qemu_sem_destroy(&p->sem); qemu_sem_destroy(&p->sem_sync); g_free(p->name); @@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p, qio_channel_set_delay(ioc, false); migration_ioc_register_yank(ioc); - p->registered_yank = true; /* Setup p->c only if the channel is completely setup */ p->c = ioc;