Re: [PATCH v4 16/19] migration: Enable TLS for preempt channel

[Peter Xu <peterx@redhat.com>]

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 <peterx@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