Re: [patch] poll() return value is actually that of select()
On Dec 12 23:12, Craig MacGregor wrote:
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:
Oh, right. I didn't check outside of the cygwin subdir. Hmpf.
Thanks, I'll applied a fix.
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 :)
I applied a (IMHO) simpler solution which doesn't require any new
variable, along these lines:
Index: poll.cc
===
RCS file: /cvs/src/src/winsup/cygwin/poll.cc,v
retrieving revision 1.48
diff -u -p -b -r1.48 poll.cc
--- poll.cc 31 Jul 2006 14:27:56 - 1.48
+++ poll.cc 13 Dec 2007 10:37:54 -
@@ -1,6 +1,6 @@
/* poll.cc. Implements poll(2) via usage of select(2) call.
- Copyright 2000, 2001, 2002, 2003, 2004, 2005, 2006 Red Hat, Inc.
+ Copyright 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007 Red Hat, Inc.
This file is part of Cygwin.
@@ -76,9 +76,14 @@ 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);
+ if (ret = 0)
+return ret;
- if (ret 0)
+ /* Set revents fields and count fds with non-zero revents fields for
+ return value. */
+ ret = 0;
for (unsigned int i = 0; i nfds; ++i)
{
if (fds[i].fd = 0)
@@ -112,6 +117,8 @@ poll (struct pollfd *fds, nfds_t nfds, i
fds[i].revents |= POLLPRI;
}
}
+ if (fds[i].revents)
+ ++ret;
}
}
The actual patch is just bigger due to the changed indentation. Please
have a look if I didn't miss anything.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Project Co-Leader cygwin AT cygwin DOT com
Red Hat
Re: [patch] poll() return value is actually that of select()
On Dec 12 16:06, zooko wrote: By the way, there is currently a patch pending in Python to work-around this bug in cygwin poll [1]. If you guys are accepting this patch to fix cygwin poll then I'll let the python developers know that. As soon as Craig made a sanity check of my solution, I'll apply the patch to 1.5.25 as well. Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
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
Re: [patch] poll() return value is actually that of select()
On Dec 13 12:09, Craig MacGregor wrote: 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 :) Thanks for your feedback. I've applied it to the cr-0x5f1 branch as well now, so it will be in 1.5.25. Thanks again, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
[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 12:59, Craig MacGregor wrote: 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. Works for me. How does it break the build for you? Patch? 2007-12-12 Craig MacGregor [EMAIL PROTECTED] * poll.cc (poll): Return count of fds with events instead of total event count 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. 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 :} Thanks again for the patch, Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
Re: [patch] poll() return value is actually that of select()
By the way, there is currently a patch pending in Python to work- around this bug in cygwin poll [1]. If you guys are accepting this patch to fix cygwin poll then I'll let the python developers know that. Regards, Zooko [1] http://bugs.python.org/issue1759997
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
