| Message ID | 5127CC3F.5030703@siemens.com |
|---|---|
| State | New |
| Headers | show |
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 <jan.kiszka@siemens.com> > --- > > 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?...) Of course Stefan should check :) The patch looks good to me. It surely can't hurt! Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thanks Laszlo
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 <jan.kiszka@siemens.com> > > --- > > > > 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.
On Fri, Feb 22, 2013 at 08:51:27PM +0100, 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 <jan.kiszka@siemens.com> > --- > > 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(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Jan Kiszka <jan.kiszka@siemens.com> writes: > Otherwise we may start processing sockets in slirp_pollfds_poll that > were created past slirp_pollfds_fill. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> I think there's still something wrong... Somehow, my char flow series breaks slirp. Prior to the g_poll conversion, this was not the case. I'll look into it more later and post a git tree but FYI. I suspect moving the chardevs to GIOChannel is uncovering a latent bug in the slirp main loop interaction. Regards, Anthony Liguori > --- > > 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); > } > -- > 1.7.3.4
On 2013-02-26 23:18, Anthony Liguori wrote: > Jan Kiszka <jan.kiszka@siemens.com> writes: > >> Otherwise we may start processing sockets in slirp_pollfds_poll that >> were created past slirp_pollfds_fill. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > I think there's still something wrong... > > Somehow, my char flow series breaks slirp. Prior to the g_poll > conversion, this was not the case. > > I'll look into it more later and post a git tree but FYI. I suspect > moving the chardevs to GIOChannel is uncovering a latent bug in the > slirp main loop interaction. OK, but please pull this fix nevertheless. It is correct and makes current git master usable again. Thanks, Jan
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); }
Otherwise we may start processing sockets in slirp_pollfds_poll that were created past slirp_pollfds_fill. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- 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(-)