On Wed, Feb 28, 2018 at 08:52:16PM +0800, Peter Xu wrote:
> 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 <pet...@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?
Yes, I don't see any reason to allow GMainContext to be changed separately
from setting the callback functions. The caller can easily just call
qio_net_listener_set_client_func_full() directly with the new GMainContext
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
