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);
+
/**
* qio_net_listener_add:
* @listener: the network listener object
diff --git a/io/net-listener.c b/io/net-listener.c
index 7f07a81fed..7ffad72f55 100644
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -145,6 +145,13 @@ static void qio_net_listener_sources_update(QIONetListener *listener,
}
}
+void qio_net_listener_set_context(QIONetListener *listener,
+ GMainContext *context)
+{
+ qio_net_listener_sources_clear(listener);
+ qio_net_listener_sources_update(listener, context);
+}
+
void qio_net_listener_add(QIONetListener *listener,
QIOChannelSocket *sioc)
{
--
2.14.3
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
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 :|
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
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 <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?
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 :|
© 2016 - 2025 Red Hat, Inc.