Re: [Qemu-devel] [PATCH v3 05/10] slirp: switch to GPollFD

[Stefan Hajnoczi]

On Wed, Feb 06, 2013 at 01:59:25AM +0100, Laszlo Ersek wrote:
> comments in-line
> 
> On 02/04/13 13:12, Stefan Hajnoczi wrote:
> > @@ -475,15 +490,15 @@ void slirp_select_poll(fd_set *readfds, fd_set 
> > *writefds, fd_set *xfds,
> >                  /*
> >                   * 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);
> >                  }
> 
> According to SUSv3+, a bit/fd set in xfds means "socket level error
> pending, or socket-specific exceptional condition". In other words,
> G_IO_ERR|G_IO_PRI. Therefore this change looks a bit suspicious, because
> a pending socket-level error would trigger this branch before the patch,
> but trickle down after the patch.
> 
> To test this, I've written the attached small test program. It hangs on
> RHEL-6.3 (2.6.32-279.19.1.el6.x86_64), which means that
> - RHEL-6.3 select() is not SUSv3/SUSv4-conformant, and
> - the mapping you mention in the commit message, and implement here, is
> correct in practice.
> 
> (As a sanity check for the test program, if you pass in the fdset as
> "writefds", the pending error is signalled & fetched OK.)

Confirmed here on Linux 3.7.4-204.fc18.x86_64.  Linux doesn't set xfds
bits on connect error.

Thanks for the test-case.

> >                  /*
> >                   * 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
> >                       */
> > @@ -502,7 +517,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
> >                       */
> 
> Looking at nothing but the loop, SS_NOFDREF appears impossible here;
> however soread() --> sofcantrcvmore() can set it after we pass the
> initial check.
> 
> Ugh. This is where sofcantrcvmore() / sofcantsendmore() show their split
> personalities.
> 
> The second part of each seems OK. They both try to set the
> SS_FCANTxxxxMORE bit corresponding to their names (meaning, that channel
> is shut down). If they find that the *other* direction has been already
> shut down, they set SS_NOFDREF instead. Fine.
> 
> The shutdown() direction is also correct in each, SHUT_RD==0 in
> sofcantrcvmore(), SHUT_WR==1 in the other. Fine again.
> 
> What is baffling is why the other direction's fd_set(s) is (are) pruned!
> git-blame doesn't help, it dates back to slirp's initial commit in qemu.
> I'm intrigued.
> 
> Anyway, if we'd like to remain bug-compatible, then
> 
>                 if (!(so->so_state & (SS_NOFDREF | SS_FCANTRCVMORE)) &&
>                         (revents & (G_IO_OUT | G_IO_ERR))) {
> 
> would be necessary here. This drags the bug into sunlight (why shouldn't
> we try to send if we read EOF?), but I believe this is what covers both
> of the "so_state" branches in sofcantrcvmore().
> 
> ... Except, of course, this would permanently kill our ability to send
> back on the half-closed socket. sofcantrcvmore() clears the write-set
> bit only for the current iteration; the next iteration will set it up
> again. Whereas SS_FCANTRCVMORE is permanent, once set.
> 
> Seems like we can't copy the original behavior here and have no choice
> but fixing this slirp bug. Shudder!

Yep, exactly.

> > @@ -588,9 +604,18 @@ 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);
> >                  }
> >              }
> 
> I think (so->s != -1) is constant true here *if* the "revents"
> expression evals to non-zero.
> - We know that "revents" has at least one interesting bit set,
> - that means (pollfds_idx != -1)
> - that means -- see slirp_select_fill() conversion -- that
>   (pfd.fd == so->s && so->s != -1)
> 
> But it doesn't hurt.

I'm a little cautious about these changes so I kept the so->s != -1
check.  Mainly because I don't know the slirp code well enough.

> >     }
> >     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 */
> 
> (I generate hunks for header files before those for C files in my
> patches, see the "-O<orderfile>" option of git-format-patch.)

Neat, thanks for sharing.  I wondered recently if it was possible to put
headers before C files in patches.