Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext

[Peter Xu <peterx@redhat.com>]

On Wed, Feb 28, 2018 at 09:25:11AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> > TCP chardevs can be using QIO network listeners working in the
> > background when in listening mode.  However the network listeners are
> > always running in main context.  This can race with chardevs that are
> > running in non-main contexts.
> > 
> > To solve this: firstly introduce qio_net_listener_set_context() to allow
> > caller to set gcontext for network listeners.  Then call it in
> > tcp_chr_update_read_handler(), with the newly cached gcontext.
> > 
> > It's fairly straightforward after we have introduced some net listener
> > helper functions - basically we unregister the GSources and add them
> > back with the correct context.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  chardev/char-socket.c     |  9 +++++++++
> >  include/io/net-listener.h | 12 ++++++++++++
> >  io/net-listener.c         |  7 +++++++
> >  3 files changed, 28 insertions(+)
> > 
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 43a2cc2c1c..8f0935cd15 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> >  
> > +    if (s->listener) {
> > +        /*
> > +         * It's possible that chardev context is changed in
> > +         * qemu_chr_be_update_read_handlers().  Reset it for QIO net
> > +         * listener if there is.
> > +         */
> > +        qio_net_listener_set_context(s->listener, chr->gcontext);
> > +    }
> > +
> >      if (!s->connected) {
> >          return;
> >      }
> > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > index 566be283b3..39dede9d6f 100644
> > --- a/include/io/net-listener.h
> > +++ b/include/io/net-listener.h
> > @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
> >                                 SocketAddress *addr,
> >                                 Error **errp);
> >  
> > +/**
> > + * qio_net_listener_set_context:
> > + * @listener: the net listener object
> > + * @context: the context that we'd like to bind the sources to
> > + *
> > + * This helper does not do anything but moves existing net listener
> > + * sources from the old one to the new one.  It can be seen as a
> > + * no-operation if there is no listening source at all.
> > + */
> > +void qio_net_listener_set_context(QIONetListener *listener,
> > +                                  GMainContext *context);
> 
> I don't think this is a good design. The GMainContext should be provided
> to the qio_net_listener_set_client_func method immediately, not updated
> after the fact

After the patches, this is qio_net_listener_set_client_func_full():

void qio_net_listener_set_client_func_full(QIONetListener *listener,
                                           QIONetListenerClientFunc func,
                                           gpointer data,
                                           GDestroyNotify notify,
                                           GMainContext *context)
{
    if (listener->io_notify) {
        listener->io_notify(listener->io_data);
    }
    listener->io_func = func;
    listener->io_data = data;
    listener->io_notify = notify;

    qio_net_listener_sources_clear(listener);
    qio_net_listener_sources_update(listener, context);
}

This is qio_net_listener_set_context():

void qio_net_listener_set_context(QIONetListener *listener,
                                  GMainContext *context)
{
    qio_net_listener_sources_clear(listener);
    qio_net_listener_sources_update(listener, context);
}

So... Basically qio_net_listener_set_context() is just a simplified
version of qio_net_listener_set_client_func_full(), but without
passing in the funcs again.  So do you mean that I can just avoid
introducing this function and call
qio_net_listener_set_client_func_full() directly?

Thanks,

-- 
Peter Xu