Re: [Qemu-devel] [PATCH] slirp: Properly initialize pollfds_idx of new sockets

[Stefan Hajnoczi]

On Fri, Feb 22, 2013 at 09:17:10PM +0100, Laszlo Ersek wrote:
> On 02/22/13 20:51, Jan Kiszka wrote:
> > Otherwise we may start processing sockets in slirp_pollfds_poll that
> > were created past slirp_pollfds_fill.
> > 
> > Signed-off-by: Jan Kiszka <address@hidden>
> > ---
> > 
> > Not sure if this pattern also applies to other users besides slirp.
> > Worth checking I suppose.
> > 
> >  slirp/socket.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/slirp/socket.c b/slirp/socket.c
> > index a7ab933..bb639ae 100644
> > --- a/slirp/socket.c
> > +++ b/slirp/socket.c
> > @@ -51,6 +51,7 @@ socreate(Slirp *slirp)
> >      so->so_state = SS_NOFDREF;
> >      so->s = -1;
> >      so->slirp = slirp;
> > +    so->pollfds_idx = -1;
> >    }
> >    return(so);
> >  }
> > 
> 
> Aaaargh. slirp_pollfds_fill() actually has three places where it does this:
> - when appending a TCP socket,
> - when appending a UDP socket,
> - when appending an ICMP socket,
> to the pollfds array.
> 
> The assumption was (I think!) that once we've iterated over all of:
> - slirp->tcb
> - slirp->udb
> - slirp->icmp
> 
> of each "slirp_instance", then we must have covered all slirp sockets in
> existence, in slirp_pollfds_fill().
> 
> I *guess* that, after we g_poll()ed, and are in slirp_pollfds_poll(), we
> can create (even accept maybe?) new sockets that are put on slirp->tcb /
> slirp->udb / slirp->icmp. That is, near the end of each of these control
> block lists, we can encounter sockets that weren't there when we
> *entered* slirp_pollfds_poll(), let alone when we called
> slirp_pollfds_fill() most recently!
> 
> (It would be interesting to see an actual call chain when this happens.)
> 
> aio-posix and iohandler seem to get this right though, I believe:
> - in aio_set_fd_handler(), the index is set to -1,
> - qemu_iohandler_fill() does the same.
> 
> (We even might have called it "general hygiene" or some such?...)

Right, we take care to initialize pollfds_idx = -1 in iohandler.c and
aio-posix.c.

Jan: Thanks for finding and fixing this, it was an oversight on my part.