On Wed, 17 Apr 2002, Martin Wilck wrote:
- In the code you're talking about, there is a potential mutual recursion
because sock_get_poll_events calls sock_reselect, which may in turn call sock_get_poll_events.
sock_get_poll_events does not call sock_reselect in the code I'm referring to, I see no reason it should. Its sole purpose is to return the event mask that should be polled next, it should not effectuate this mask. Or are you talking about something else? Hmm, sounds like you may be talking about sock_poll_event... ok, let's see...
I understand that infinite recursion is prevented by handling the sock->hmask field, but I wonder whether it is necessary at all that sock_get_poll_events calls sock_reselect() and not only set_select_events(sock_get_poll_events()) ?
The event mask to check for may change during sock_poll_event and it would be nice to check for those new events immediately. But at the moment I don't recall any situation where it would be necessary.
I'd prefer a solution where no call sequence sock_get_poll_events->sock_reselect->sock_get_poll_events->... would be possible.
IMO, the code snippet
if (event & POLLIN) { char dummy; /* Linux 2.4 doesn't report POLLHUP if only one side of the socket * has been closed, so we need to check for it explicitly here */ if (!recv( sock->obj.fd, &dummy, 1, MSG_PEEK )) event = POLLHUP; }[...]
if (event & (POLLERR|POLLHUP)) set_select_events( &sock->obj, -1 );is wrong, because an empty read only signals that the upstream connection was closed.
Hmm. I don't think I wrote that code piece, so I don't recall its justification. It may well be that it was ill-conceived (at least I don't quite like the looks of that code), especially since POLLHUP is the "problem" we tried to solve with all this insidious code, not its "solution". There used to be different code to detect a closed socket, but it seems it was removed for some reason (probably because it had other issues), and then maybe that thing above was hacked on top because the old code was no longer there?
The downstream connection may still be open, i.e. FD_WRITE events may well occur. In this case it seems to be wrong to do set_select_events( &sock->obj, -1 ) _unless_ the kernel had signalled POLLHUP|POLLIN, which means that both sides of the socket have been closed. Thus, if we have an empty read but no POLLHUP from the kernel, we should signal FD_CLOSE to the app, but not pretend we had got POLLHUP as the current code does.
Right?
Yeah.