On Thu, Mar 01, 2018 at 03:43:31PM +0000, Daniel P. Berrangé wrote:
> On Thu, Mar 01, 2018 at 04:44:30PM +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, we need to re-setup the net listeners in
> > tcp_chr_update_read_handler() with the newly cached gcontext.
> >
> > Since at it, generalize a tcp_chr_net_listener_setup() helper function
> > and clean up the old code a bit.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > chardev/char-socket.c | 32 ++++++++++++++++++++++++++------
> > 1 file changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 43a2cc2c1c..5cd20cc932 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -410,6 +410,19 @@ static void update_disconnected_filename(SocketChardev
> > *s)
> > s->is_listen, s->is_telnet);
> > }
> >
> > +/* Set enable=true to start net listeners, false to stop them. */
> > +static void tcp_chr_net_listener_setup(SocketChardev *s, bool enable)
> > +{
> > + Chardev *chr = CHARDEV(s);
> > +
> > + /* Net listeners' context will follow the Chardev's. */
> > + qio_net_listener_set_client_func_full(s->listener,
> > + enable ? tcp_chr_accept : NULL,
> > + enable ? chr : NULL,
> > + NULL,
> > + chr->gcontext);
>
> I don't think this helper method is really a benefit. In fact I think
> it makes understanding the code harder, because when you see
> tcp_chr_net_listener_setup(s, true), you've no idea what 'true' means
> without going to finding the impl of tcp_chr_net_listener_setup().
>
> Just leave the direct calls to qio_net_listener_set_client_func_full
> as they are IMHO.
Frankly speaking I was a bit confused when I started to read
chardev/qio codes with so many hooks, e.g., when I saw:
qio_net_listener_set_client_func(s->listener, tcp_chr_accept,
chr, NULL);
I totally have no idea on what happened. I need to go deeper into the
net listener code to know that, hmm, it's setting up something to
accept connections!
If I can have something like:
tcp_chr_net_listener_setup(s, true);
It may be easier for me to understand that there's something either
registered for the listening ports, and I don't need to care about
which function will be called when accept happened. Basically it
"hides" some logic inside, that's IMHO where functions/macros help.
(Here the naming of function is discussible for sure, along with how
to define the parameters)
I think it may be a flavor issue. In that case, I'm always fine with
either way. I assume the previous cleanup patch 5 is similarly a
flavor issue too, so I'll follow your final judgement on what you
would prefer.
Thanks,
--
Peter Xu
