Re: [patch] poll() return value is actually that of select()
On Dec 13, 2007 5:59 AM, Corinna Vinschen [EMAIL PROTECTED] wrote: As soon as Craig made a sanity check of my solution, I'll apply the patch to 1.5.25 as well. Looks good to me, works... yes your way is simpler :) -craig
[patch] poll() return value is actually that of select()
bug example src: http://pastebin.com/f719e4c11 First off... I've been happily using Cygwin for years, glad I finally have a patch to contribute...Thanks! While testing some socket code on Cygwin and Linux, I noticed the returns from poll() were always twice what they were on Linux when a socket had been closed remotely and then polled(). From 'man 2 poll' on debian: On success, a positive number is returned; this is the number of structures which have non-zero revents fields (in other words, those descriptors with events or errors reported). A value of 0 indicates that the call timed out and no file descriptors were ready. On error, -1 is returned, and errno is set appropriately. Digging deeper, I found that Cygwin's poll() is implemented with cygwin_select(), and thus returns the total number of ISSET events, instead of just the number of fds with events. Thus, the behavior is: On success, select() and pselect() return the number of file descriptors contained in the three returned descriptor sets (that is, the total number of bits that are set in readfds, writefds, exceptfds) which may be zero if the timeout expires before anything interesting happens. On error, -1 is returned, and errno is set appropriately; the sets and timeout become undefined, so do not rely on their contents after an error. Attached is some goo which makes poll() work as expected compiled, tested, works... fyi, as of 9:30am EST string.h broke the build, i had to roll it back. -craig 2007-12-12 Craig MacGregor [EMAIL PROTECTED] * poll.cc (poll): Return count of fds with events instead of total event count Index: cygwin/poll.cc === RCS file: /cvs/src/src/winsup/cygwin/poll.cc,v retrieving revision 1.48 diff -u -p -r1.48 poll.cc --- cygwin/poll.cc 31 Jul 2006 14:27:56 - 1.48 +++ cygwin/poll.cc 12 Dec 2007 13:29:53 - @@ -76,15 +76,18 @@ poll (struct pollfd *fds, nfds_t nfds, i if (invalid_fds) return invalid_fds; - int ret = cygwin_select (max_fd + 1, read_fds, write_fds, except_fds, timeout 0 ? NULL : tv); + int ret = cygwin_select (max_fd + 1, read_fds, write_fds, except_fds, timeout 0 ? NULL : tv); + + int ir = 0, ret_p = 0; +/* Count fds in ISSETs for return, but just once each */ if (ret 0) -for (unsigned int i = 0; i nfds; ++i) +for (unsigned int i = 0; i nfds; ret_p+=ir, ir = 0, ++i) { if (fds[i].fd = 0) { if (cygheap-fdtab.not_open (fds[i].fd)) - fds[i].revents = POLLHUP; + ir = 1, fds[i].revents = POLLHUP; else { fhandler_socket *sock; @@ -98,22 +101,23 @@ poll (struct pollfd *fds, nfds_t nfds, i will return -1 with errno set to the appropriate value. So it looks like there's actually no good reason to return POLLERR. */ - fds[i].revents |= POLLIN; + ir = 1, fds[i].revents |= POLLIN; /* Handle failed connect. */ if (FD_ISSET(fds[i].fd, write_fds) (sock = cygheap-fdtab[fds[i].fd]-is_socket ()) sock-connect_state () == connect_failed) - fds[i].revents |= (POLLIN | POLLERR); + ir = 1, fds[i].revents |= (POLLIN | POLLERR); else { if (FD_ISSET(fds[i].fd, write_fds)) - fds[i].revents |= POLLOUT; + ir = 1, fds[i].revents |= POLLOUT; if (FD_ISSET(fds[i].fd, except_fds)) - fds[i].revents |= POLLPRI; + ir = 1, fds[i].revents |= POLLPRI; } } - } + } + } - return ret; + return !ret_p ? ret : ret_p; }
Re: [patch] poll() return value is actually that of select()
On Dec 12, 2007 1:57 PM, Corinna Vinschen wrote: Works for me. How does it break the build for you? Patch? I get the following error making cygserver... i set up my dev env in a rush and just wanted a clean build, so i rolled back string.h to 1.8 for a quick fix: g++ -L/cyg/build/i686-pc-cygwin/winsup -L/cyg/build/i686-pc-cygwin/winsup/cygwin -L/cyg/build/i686-pc-cygwin/winsup/w32api/lib -isystem /cyg/src/winsup/include -isystem /cyg/src/winsup/cygwin/include -isystem /cyg/src/winsup/w32api/include -B/cyg/build/i686-pc-cygwin/newlib/ -isystem /cyg/build/i686-pc-cygwin/newlib/targ-include -isystem /cyg/src/newlib/libc/include -o cygserver.exe cygserver.o client.o process.o msg.o sem.o shm.o threaded_queue.o transport.o transport_pipes.o bsd_helper.o bsd_log.o bsd_mutex.o sysv_msg.o sysv_sem.o sysv_shm.o /cyg/build/i686-pc-cygwin/winsup/cygwin/smallprint.o /cyg/build/i686-pc-cygwin/winsup/cygwin/strfuncs.o /cyg/build/i686-pc-cygwin/winsup/cygwin/version.o -L/cyg/build/i686-pc-cygwin/winsup/cygwin -lntdll bsd_helper.o: In function `_Z17default_tun_checkP10tun_structPcPKc': /cyg/src/winsup/cygserver/bsd_helper.cc:525: undefined reference to [EMAIL PROTECTED]' /cyg/src/winsup/cygserver/bsd_helper.cc:525: undefined reference to [EMAIL PROTECTED]' /cyg/src/winsup/cygserver/bsd_helper.cc:525: undefined reference to [EMAIL PROTECTED]' /cyg/src/winsup/cygserver/bsd_helper.cc:525: undefined reference to [EMAIL PROTECTED]' /cyg/src/winsup/cygserver/bsd_helper.cc:525: undefined reference to [EMAIL PROTECTED]' bsd_helper.o:/cyg/src/winsup/cygserver/bsd_helper.cc:529: more undefined references to [EMAIL PROTECTED]' follow Info: resolving __ctype_ by linking to __imp___ctype_ (auto-import) collect2: ld returned 1 exit status make[3]: *** [cygserver.exe] Error 1 make[3]: Leaving directory `/cyg/build/i686-pc-cygwin/winsup/cygserver' make[2]: *** [cygserver] Error 1 make[2]: Leaving directory `/cyg/build/i686-pc-cygwin/winsup' make[1]: *** [all-target-winsup] Error 2 make[1]: Leaving directory `/cyg/build' make: *** [all] Error 2 Thanks for the patch. It looks good to me, but I'll slightly reformat it. I'll rather have the `ir = 1' expressions standalone on a single line and curly brackets. I'll apply it tomorrow. I changed as few lines as possible to avoid the next point :) However, this patch is already almost beyond the upper bound (in terms of patch size) which we can incorporate without having a signed copyright assignment from you, see http://cygwin.com/contrib.html, section Before you get started. I don't want to keep you from providing more and bigger patches, of course, but we all had to go through this legal stuff :} I'll make a note to myself to get an agreement out... starting a new job so I don't know how many more patches I'll be sending in, but it can't hurt to be ready I suppose. -craig