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.
