On Fri, Feb 1, 2013 at 1:53 PM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
> Slirp uses rfds/wfds/xfds more extensively than other QEMU components.
>
> The rarely-used out-of-band TCP data feature is used. That means we
> need the full table of select(2) to g_poll(3) events:
>
> rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR
> wfds -> G_IO_OUT | G_IO_ERR
> xfds -> G_IO_PRI
>
> I came up with this table by looking at Linux fs/select.c which maps
> select(2) to poll(2) internally.
>
> Another detail to watch out for are the global variables that reference
> rfds/wfds/xfds during slirp_select_poll(). sofcantrcvmore() and
> sofcantsendmore() use these globals to clear fd_set bits. When
> sofcantrcvmore() is called, the wfds bit is cleared so that the write
> handler will no longer be run for this iteration of the event loop.
>
> This actually seems buggy to me since TCP connections can be half-closed
> and we'd still want to handle data in half-duplex fashion. I think the
> real intention is to avoid running the read/write handler when the
> socket has been fully closed. This is indicated with the SS_NOFDREF
> state bit so we now check for it before invoking the TCP write handler.
> Note that UDP/ICMP code paths don't care because they are
> connectionless.
>
> Note that slirp/ has a lot of tabs and sometimes mixed tabs with spaces.
> I followed the style of the surrounding code.
Nack for CODING_STYLE. Please either convert the functions affected to
spaces first (as you yourself proposed), or just ignore the
surrounding code and use spaces. Read my lips: no new tabses.
>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> main-loop.c | 4 +-
> slirp/libslirp.h | 6 +--
> slirp/main.h | 1 -
> slirp/slirp.c | 136
> +++++++++++++++++++++++++++++++++----------------------
> slirp/socket.c | 9 ----
> slirp/socket.h | 2 +
> stubs/slirp.c | 6 +--
> 7 files changed, 89 insertions(+), 75 deletions(-)
>
> diff --git a/main-loop.c b/main-loop.c
> index 12b0213..49e97ff 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -503,13 +503,13 @@ int main_loop_wait(int nonblocking)
>
> #ifdef CONFIG_SLIRP
> slirp_update_timeout(&timeout);
> - slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
> + slirp_pollfds_fill(gpollfds);
> #endif
> qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds);
> ret = os_host_main_loop_wait(timeout);
> qemu_iohandler_poll(&rfds, &wfds, &xfds, ret);
> #ifdef CONFIG_SLIRP
> - slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
> + slirp_pollfds_poll(gpollfds, (ret < 0));
> #endif
>
> qemu_run_all_timers();
> diff --git a/slirp/libslirp.h b/slirp/libslirp.h
> index 49609c2..ceabff8 100644
> --- a/slirp/libslirp.h
> +++ b/slirp/libslirp.h
> @@ -17,11 +17,9 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
> void slirp_cleanup(Slirp *slirp);
>
> void slirp_update_timeout(uint32_t *timeout);
> -void slirp_select_fill(int *pnfds,
> - fd_set *readfds, fd_set *writefds, fd_set *xfds);
> +void slirp_pollfds_fill(GArray *pollfds);
>
> -void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
> - int select_error);
> +void slirp_pollfds_poll(GArray *pollfds, int select_error);
>
> void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len);
>
> diff --git a/slirp/main.h b/slirp/main.h
> index 66e4f92..f2e58cf 100644
> --- a/slirp/main.h
> +++ b/slirp/main.h
> @@ -31,7 +31,6 @@ extern int ctty_closed;
> extern char *slirp_tty;
> extern char *exec_shell;
> extern u_int curtime;
> -extern fd_set *global_readfds, *global_writefds, *global_xfds;
> extern struct in_addr loopback_addr;
> extern unsigned long loopback_mask;
> extern char *username;
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 0e6e232..967b836 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -39,9 +39,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = {
>
> static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 };
>
> -/* XXX: suppress those select globals */
> -fd_set *global_readfds, *global_writefds, *global_xfds;
> -
> u_int curtime;
> static u_int time_fasttimo, last_slowtimo;
> static int do_slowtimo;
> @@ -261,7 +258,6 @@ void slirp_cleanup(Slirp *slirp)
>
> #define CONN_CANFSEND(so) (((so)->so_state &
> (SS_FCANTSENDMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED)
> #define CONN_CANFRCV(so) (((so)->so_state &
> (SS_FCANTRCVMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED)
> -#define UPD_NFDS(x) if (nfds < (x)) nfds = (x)
>
> void slirp_update_timeout(uint32_t *timeout)
> {
> @@ -270,23 +266,15 @@ void slirp_update_timeout(uint32_t *timeout)
> }
> }
>
> -void slirp_select_fill(int *pnfds,
> - fd_set *readfds, fd_set *writefds, fd_set *xfds)
> +void slirp_pollfds_fill(GArray *pollfds)
> {
> Slirp *slirp;
> struct socket *so, *so_next;
> - int nfds;
>
> if (QTAILQ_EMPTY(&slirp_instances)) {
> return;
> }
>
> - /* fail safe */
> - global_readfds = NULL;
> - global_writefds = NULL;
> - global_xfds = NULL;
> -
> - nfds = *pnfds;
> /*
> * First, TCP sockets
> */
> @@ -302,8 +290,12 @@ void slirp_select_fill(int *pnfds,
>
> for (so = slirp->tcb.so_next; so != &slirp->tcb;
> so = so_next) {
> + int events = 0;
> +
> so_next = so->so_next;
>
> + so->pollfds_idx = -1;
> +
> /*
> * See if we need a tcp_fasttimo
> */
> @@ -321,8 +313,12 @@ void slirp_select_fill(int *pnfds,
> * Set for reading sockets which are accepting
> */
> if (so->so_state & SS_FACCEPTCONN) {
> - FD_SET(so->s, readfds);
> - UPD_NFDS(so->s);
> + GPollFD pfd = {
> + .fd = so->s,
> + .events = G_IO_IN | G_IO_HUP | G_IO_ERR,
> + };
> + so->pollfds_idx = pollfds->len;
> + g_array_append_val(pollfds, pfd);
> continue;
> }
>
> @@ -330,8 +326,12 @@ void slirp_select_fill(int *pnfds,
> * Set for writing sockets which are connecting
> */
> if (so->so_state & SS_ISFCONNECTING) {
> - FD_SET(so->s, writefds);
> - UPD_NFDS(so->s);
> + GPollFD pfd = {
> + .fd = so->s,
> + .events = G_IO_OUT | G_IO_ERR,
> + };
> + so->pollfds_idx = pollfds->len;
> + g_array_append_val(pollfds, pfd);
> continue;
> }
>
> @@ -340,8 +340,7 @@ void slirp_select_fill(int *pnfds,
> * we have something to send
> */
> if (CONN_CANFSEND(so) && so->so_rcv.sb_cc) {
> - FD_SET(so->s, writefds);
> - UPD_NFDS(so->s);
> + events |= G_IO_OUT | G_IO_ERR;
> }
>
> /*
> @@ -349,9 +348,16 @@ void slirp_select_fill(int *pnfds,
> * receive more, and we have room for it XXX /2 ?
> */
> if (CONN_CANFRCV(so) && (so->so_snd.sb_cc <
> (so->so_snd.sb_datalen/2))) {
> - FD_SET(so->s, readfds);
> - FD_SET(so->s, xfds);
> - UPD_NFDS(so->s);
> + events |= G_IO_IN | G_IO_HUP | G_IO_ERR |
> G_IO_PRI;
> + }
> +
> + if (events) {
> + GPollFD pfd = {
> + .fd = so->s,
> + .events = events,
> + };
> + so->pollfds_idx = pollfds->len;
> + g_array_append_val(pollfds, pfd);
> }
> }
>
> @@ -362,6 +368,8 @@ void slirp_select_fill(int *pnfds,
> so = so_next) {
> so_next = so->so_next;
>
> + so->pollfds_idx = -1;
> +
> /*
> * See if it's timed out
> */
> @@ -384,8 +392,12 @@ void slirp_select_fill(int *pnfds,
> * (XXX <= 4 ?)
> */
> if ((so->so_state & SS_ISFCONNECTED) && so->so_queued
> <= 4) {
> - FD_SET(so->s, readfds);
> - UPD_NFDS(so->s);
> + GPollFD pfd = {
> + .fd = so->s,
> + .events = G_IO_IN | G_IO_HUP | G_IO_ERR,
> + };
> + so->pollfds_idx = pollfds->len;
> + g_array_append_val(pollfds, pfd);
> }
> }
>
> @@ -396,6 +408,8 @@ void slirp_select_fill(int *pnfds,
> so = so_next) {
> so_next = so->so_next;
>
> + so->pollfds_idx = -1;
> +
> /*
> * See if it's timed out
> */
> @@ -409,17 +423,18 @@ void slirp_select_fill(int *pnfds,
> }
>
> if (so->so_state & SS_ISFCONNECTED) {
> - FD_SET(so->s, readfds);
> - UPD_NFDS(so->s);
> + GPollFD pfd = {
> + .fd = so->s,
> + .events = G_IO_IN | G_IO_HUP | G_IO_ERR,
> + };
> + so->pollfds_idx = pollfds->len;
> + g_array_append_val(pollfds, pfd);
> }
> }
> }
> -
> - *pnfds = nfds;
> }
>
> -void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds,
> - int select_error)
> +void slirp_pollfds_poll(GArray *pollfds, int select_error)
> {
> Slirp *slirp;
> struct socket *so, *so_next;
> @@ -429,10 +444,6 @@ void slirp_select_poll(fd_set *readfds, fd_set
> *writefds, fd_set *xfds,
> return;
> }
>
> - global_readfds = readfds;
> - global_writefds = writefds;
> - global_xfds = xfds;
> -
> curtime = qemu_get_clock_ms(rt_clock);
>
> QTAILQ_FOREACH(slirp, &slirp_instances, entry) {
> @@ -458,26 +469,32 @@ void slirp_select_poll(fd_set *readfds, fd_set
> *writefds, fd_set *xfds,
> */
> for (so = slirp->tcb.so_next; so != &slirp->tcb;
> so = so_next) {
> + int revents;
> +
> so_next = so->so_next;
>
> - /*
> - * FD_ISSET is meaningless on these sockets
> - * (and they can crash the program)
> - */
> - if (so->so_state & SS_NOFDREF || so->s == -1)
> + revents = 0;
> + if (so->pollfds_idx != -1) {
> + revents = g_array_index(pollfds, GPollFD,
> +
> so->pollfds_idx).revents;
> + }
> +
> + if (so->so_state & SS_NOFDREF || so->s == -1) {
> continue;
> + }
>
> /*
> * Check for URG data
> * This will soread as well, so no need to
> - * test for readfds below if this succeeds
> + * test for G_IO_IN below if this succeeds
> */
> - if (FD_ISSET(so->s, xfds))
> + if (revents & G_IO_PRI) {
> sorecvoob(so);
> + }
> /*
> * Check sockets for reading
> */
> - else if (FD_ISSET(so->s, readfds)) {
> + else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) {
> /*
> * Check for incoming connections
> */
> @@ -495,7 +512,8 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds,
> fd_set *xfds,
> /*
> * Check sockets for writing
> */
> - if (FD_ISSET(so->s, writefds)) {
> + if (!(so->so_state & SS_NOFDREF) &&
> + (revents & (G_IO_OUT | G_IO_ERR))) {
> /*
> * Check for non-blocking, still-connecting sockets
> */
> @@ -576,9 +594,17 @@ void slirp_select_poll(fd_set *readfds, fd_set
> *writefds, fd_set *xfds,
> */
> for (so = slirp->udb.so_next; so != &slirp->udb;
> so = so_next) {
> + int revents;
> +
> so_next = so->so_next;
>
> - if (so->s != -1 && FD_ISSET(so->s, readfds)) {
> + revents = 0;
> + if (so->pollfds_idx != -1) {
> + revents = g_array_index(pollfds, GPollFD,
> +
> so->pollfds_idx).revents;
> + }
> +
> + if (so->s != -1 && (revents & (G_IO_IN | G_IO_HUP |
> G_IO_ERR))) {
> sorecvfrom(so);
> }
> }
> @@ -588,9 +614,18 @@ void slirp_select_poll(fd_set *readfds, fd_set
> *writefds, fd_set *xfds,
> */
> for (so = slirp->icmp.so_next; so != &slirp->icmp;
> so = so_next) {
> - so_next = so->so_next;
> + int revents;
>
> - if (so->s != -1 && FD_ISSET(so->s, readfds)) {
> + so_next = so->so_next;
> +
> + revents = 0;
> + if (so->pollfds_idx != -1) {
> + revents = g_array_index(pollfds, GPollFD,
> + so->pollfds_idx).revents;
> + }
> +
> + if (so->s != -1 &&
> + (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) {
> icmp_receive(so);
> }
> }
> @@ -598,15 +633,6 @@ void slirp_select_poll(fd_set *readfds, fd_set
> *writefds, fd_set *xfds,
>
> if_start(slirp);
> }
> -
> - /* clear global file descriptor sets.
> - * these reside on the stack in vl.c
> - * so they're unusable if we're not in
> - * slirp_select_fill or slirp_select_poll.
> - */
> - global_readfds = NULL;
> - global_writefds = NULL;
> - global_xfds = NULL;
> }
>
> static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 77b0c98..a7ab933 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -680,9 +680,6 @@ sofcantrcvmore(struct socket *so)
> {
> if ((so->so_state & SS_NOFDREF) == 0) {
> shutdown(so->s,0);
> - if(global_writefds) {
> - FD_CLR(so->s,global_writefds);
> - }
> }
> so->so_state &= ~(SS_ISFCONNECTING);
> if (so->so_state & SS_FCANTSENDMORE) {
> @@ -698,12 +695,6 @@ sofcantsendmore(struct socket *so)
> {
> if ((so->so_state & SS_NOFDREF) == 0) {
> shutdown(so->s,1); /* send FIN to fhost */
> - if (global_readfds) {
> - FD_CLR(so->s,global_readfds);
> - }
> - if (global_xfds) {
> - FD_CLR(so->s,global_xfds);
> - }
> }
> so->so_state &= ~(SS_ISFCONNECTING);
> if (so->so_state & SS_FCANTRCVMORE) {
> diff --git a/slirp/socket.h b/slirp/socket.h
> index 857b0da..57e0407 100644
> --- a/slirp/socket.h
> +++ b/slirp/socket.h
> @@ -20,6 +20,8 @@ struct socket {
>
> int s; /* The actual socket */
>
> + int pollfds_idx; /* GPollFD GArray index */
> +
> Slirp *slirp; /* managing slirp instance */
>
> /* XXX union these with not-yet-used sbuf params */
> diff --git a/stubs/slirp.c b/stubs/slirp.c
> index 9a3309a..f1fc833 100644
> --- a/stubs/slirp.c
> +++ b/stubs/slirp.c
> @@ -5,13 +5,11 @@ void slirp_update_timeout(uint32_t *timeout)
> {
> }
>
> -void slirp_select_fill(int *pnfds, fd_set *readfds,
> - fd_set *writefds, fd_set *xfds)
> +void slirp_pollfds_fill(GArray *pollfds)
> {
> }
>
> -void slirp_select_poll(fd_set *readfds, fd_set *writefds,
> - fd_set *xfds, int select_error)
> +void slirp_pollfds_poll(GArray *pollfds, int select_error)
> {
> }
>
> --
> 1.8.1
>
>
