qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH] slirp: Properly initialize pollfds_idx of new sockets
Date: Fri, 22 Feb 2013 21:17:10 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130216 Thunderbird/17.0.3

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?...)

Of course Stefan should check :)

The patch looks good to me. It surely can't hurt!

Reviewed-by: Laszlo Ersek <address@hidden>

Thanks
Laszlo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]