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

[Peter Xu <peterx@redhat.com>]

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 <peterx@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