Re: [Qemu-devel] [PATCH 14/14] qio/chardev: specify gcontext for TLS handshake

[Peter Xu]

On Wed, Feb 28, 2018 at 01:22:37PM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:33PM +0800, Peter Xu wrote:
> > We allow the TLS code to be run with non-default gcontext by providing a
> > new qio_channel_tls_handshake_full() API.
> > 
> > With the new API, we can re-setup the TLS handshake GSource by calling
> > it again with the correct gcontext.  Any call to the function will clean
> > up existing GSource tasks, and re-setup using the new gcontext.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  chardev/char-socket.c    | 30 +++++++++++++---
> >  include/io/channel-tls.h | 22 +++++++++++-
> >  io/channel-tls.c         | 91 
> > ++++++++++++++++++++++++++++++++++++++++--------
> >  3 files changed, 123 insertions(+), 20 deletions(-)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 164a64ff34..406d33c04f 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -72,6 +72,9 @@ typedef struct {
> >  
> >  static gboolean socket_reconnect_timeout(gpointer opaque);
> >  static void tcp_chr_telnet_init(Chardev *chr);
> > +static void tcp_chr_tls_handshake_setup(Chardev *chr,
> > +                                        QIOChannelTLS *tioc,
> > +                                        GMainContext *context);
> >  
> >  static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
> >  {
> > @@ -570,6 +573,7 @@ static void tcp_chr_telnet_destroy(SocketChardev *s)
> >  static void tcp_chr_update_read_handler(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > +    QIOChannelTLS *tioc;
> >  
> >      if (s->listener) {
> >          /*
> > @@ -589,6 +593,17 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> >          qio_task_context_set(s->thread_task, chr->gcontext);
> >      }
> >  
> > +    tioc = (QIOChannelTLS *)object_dynamic_cast(OBJECT(s->ioc),
> > +                                                TYPE_QIO_CHANNEL_TLS);
> > +    if (tioc) {
> > +        /*
> > +         * TLS session enabled; reconfigure things up.  Note that, if
> > +         * there is existing handshake task, it'll be cleaned up first
> > +         * in QIO code.
> > +         */
> > +        tcp_chr_tls_handshake_setup(chr, tioc, chr->gcontext);
> > +    }
> 
> This is crazy - we should not be looking at specific implementations of
> the channel. If the TLS object needs to use a specific GMainContext we
> should make sure that is done right from the start and not try to change
> the GMainContext on the fly.

I'm not sure whether I can do it since current code has already let
the chardev frontends depend on the backends, so we cannot simply let
it be reverted (setup context basically means we need to have the
frontend be inited before backends since the context is now
frontend-specific).

However I'm thinking maybe I can postpone some of the chardev
initialization process after everything has been setup.  Then it'll
look like:

- init chardev backends, phase 1 (e.g., only create chardevs but
  postpone open)
- init chardev frontends (e.g., monitors)
- init chardev backends, phase 2 (e.g., do the real socket open work)

Actually I already spotted an existing user of it
(muxes_realize_notify).  Maybe I can do similar thing to postpone some
of the socket chardev operations after machine init finished.  Thanks,

-- 
Peter Xu