On Wed, Apr 20, 2022 at 12:35:21PM +0100, Daniel P. Berrangé wrote:
> On Thu, Mar 31, 2022 at 11:08:54AM -0400, Peter Xu wrote:
> > This patch is based on the async preempt channel creation. It continues
> > wiring up the new channel with TLS handshake to destionation when enabled.
> >
> > Note that only the src QEMU needs such operation; the dest QEMU does not
> > need any change for TLS support due to the fact that all channels are
> > established synchronously there, so all the TLS magic is already properly
> > handled by migration_tls_channel_process_incoming().
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > migration/postcopy-ram.c | 60 +++++++++++++++++++++++++++++++++++-----
> > migration/trace-events | 1 +
> > 2 files changed, 54 insertions(+), 7 deletions(-)
> >
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index ab2a50cf45..f5ba176862 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -36,6 +36,7 @@
> > #include "socket.h"
> > #include "qemu-file-channel.h"
> > #include "yank_functions.h"
> > +#include "tls.h"
> >
> > /* Arbitrary limit on size of each discard command,
> > * keeps them around ~200 bytes
> > @@ -1552,15 +1553,15 @@ bool
> > postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file)
> > return true;
> > }
> >
> > +/*
> > + * Setup the postcopy preempt channel with the IOC. If ERROR is specified,
> > + * setup the error instead. This helper will free the ERROR if specified.
> > + */
> > static void
> > -postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque)
> > +postcopy_preempt_send_channel_done(MigrationState *s,
> > + QIOChannel *ioc, Error *local_err)
> > {
> > - MigrationState *s = opaque;
> > - QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
> > - Error *local_err = NULL;
> > -
> > - if (qio_task_propagate_error(task, &local_err)) {
> > - /* Something wrong happened.. */
> > + if (local_err) {
> > migrate_set_error(s, local_err);
> > error_free(local_err);
> > } else {
> > @@ -1574,6 +1575,51 @@ postcopy_preempt_send_channel_new(QIOTask *task,
> > gpointer opaque)
> > * postcopy_qemufile_src to know whether it failed or not.
> > */
> > qemu_sem_post(&s->postcopy_qemufile_src_sem);
> > +}
> > +
> > +static void
> > +postcopy_preempt_tls_handshake(QIOTask *task, gpointer opaque)
> > +{
> > + MigrationState *s = opaque;
> > + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
>
> If using g_autoptr(QIOChannel) ioc = ...
New magic learned..
>
> > + Error *err = NULL;
>
> local_err is normal naming
OK.
>
> > +
> > + qio_task_propagate_error(task, &err);
> > + postcopy_preempt_send_channel_done(s, ioc, err);
> > + object_unref(OBJECT(ioc));
>
> ...not needed with g_autoptr
>
> > +}
> > +
> > +static void
> > +postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque)
> > +{
> > + MigrationState *s = opaque;
> > + QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
>
> If you use g_autoptr(QIOChannel)
Will use it here too.
>
> > + QIOChannelTLS *tioc;
> > + Error *local_err = NULL;
> > +
> > + if (qio_task_propagate_error(task, &local_err)) {
> > + assert(local_err);
>
> I don't think we really need to add these asserts everywhere we
> handle a failure path do we ?
Maybe I'm just over-cautious, yeah let me drop those.
Thanks,
--
Peter Xu
