tests/commandhelper.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
commandhelper hangs indefinetely in poll() on macOS on commandtest test2
and later because POLLNVAL is returned on revents for input file
descriptor opened from /dev/null, i.e this hangs:
$ tests/commandhelper < /dev/null
BEGIN STDOUT
BEGIN STDERR
^C
But it works fine with regular stdin:
$ tests/commandhelper <<< test
BEGIN STDOUT
BEGIN STDERR
test
test
END STDOUT
END STDERR
The issue is mentioned in poll(2):
BUGS
The poll() system call currently does not support devices.
With the change all 28 cases in commandtest pass.
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
tests/commandhelper.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c
index 7c260c4e13..676ef55e9f 100644
--- a/tests/commandhelper.c
+++ b/tests/commandhelper.c
@@ -190,7 +190,15 @@ int main(int argc, char **argv) {
}
for (i = 0; i < numpollfds; i++) {
- if (fds[i].revents & (POLLIN | POLLHUP | POLLERR)) {
+ if (fds[i].revents & (POLLIN | POLLHUP | POLLERR |
+# ifdef __APPLE__
+ /*
+ * poll() on /dev/null will return POLLNVAL
+ */
+ POLLNVAL)) {
+# else
+ 0)) {
+# endif
fds[i].revents = 0;
got = read(fds[i].fd, buf, sizeof(buf));
--
2.28.0
On Wed, 2020-10-07 at 15:20 +0300, Roman Bolshakov wrote:
> for (i = 0; i < numpollfds; i++) {
> - if (fds[i].revents & (POLLIN | POLLHUP | POLLERR)) {
> + if (fds[i].revents & (POLLIN | POLLHUP | POLLERR |
> +# ifdef __APPLE__
> + /*
> + * poll() on /dev/null will return POLLNVAL
> + */
> + POLLNVAL)) {
> +# else
> + 0)) {
> +# endif
The fix seems legit, but having a preprocessor directive in the
middle of a function call doesn't look great. Can you rewrite this
along the lines of
for (i = 0; i < numpollfds; i++) {
short revents = POLLIN | POLLHUP | POLLERR;
# ifdef __APPLE__
/* On macOS, poll() on devices will return POLLNVAL */
revents |= POLLNVAL;
# endif
if (fds[i].revents & revents) {
/* ... */
}
}
please?
--
Andrea Bolognani / Red Hat / Virtualization
On Wed, Oct 07, 2020 at 06:57:21PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-10-07 at 15:20 +0300, Roman Bolshakov wrote:
> > for (i = 0; i < numpollfds; i++) {
> > - if (fds[i].revents & (POLLIN | POLLHUP | POLLERR)) {
> > + if (fds[i].revents & (POLLIN | POLLHUP | POLLERR |
> > +# ifdef __APPLE__
> > + /*
> > + * poll() on /dev/null will return POLLNVAL
> > + */
> > + POLLNVAL)) {
> > +# else
> > + 0)) {
> > +# endif
>
> The fix seems legit, but having a preprocessor directive in the
> middle of a function call doesn't look great. Can you rewrite this
> along the lines of
>
> for (i = 0; i < numpollfds; i++) {
> short revents = POLLIN | POLLHUP | POLLERR;
>
> # ifdef __APPLE__
> /* On macOS, poll() on devices will return POLLNVAL */
> revents |= POLLNVAL;
> # endif
>
> if (fds[i].revents & revents) {
> /* ... */
> }
> }
>
> please?
>
Sure, this looks much better, thanks!
Thanks,
Roman
On a Wednesday in 2020, Roman Bolshakov wrote: >commandhelper hangs indefinetely in poll() on macOS on commandtest test2 *indefinitely >and later because POLLNVAL is returned on revents for input file >descriptor opened from /dev/null, i.e this hangs: > > $ tests/commandhelper < /dev/null > BEGIN STDOUT > BEGIN STDERR > ^C > >But it works fine with regular stdin: > > $ tests/commandhelper <<< test > BEGIN STDOUT > BEGIN STDERR > test > test > END STDOUT > END STDERR > >The issue is mentioned in poll(2): > > BUGS > The poll() system call currently does not support devices. > >With the change all 28 cases in commandtest pass. > Thanks for fixing this. Is there a bug that track this? If so, including it in a comment would help us delete this special case in distant future. Jano >Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> >--- > tests/commandhelper.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-)
On Wed, Oct 07, 2020 at 07:17:05PM +0200, Ján Tomko wrote: > On a Wednesday in 2020, Roman Bolshakov wrote: Hi Jano, > > *indefinitely > Thanks, I'll correct it. > > BUGS > > The poll() system call currently does not support devices. > > > > Is there a bug that track this? If so, including it in a comment > would help us delete this special case in distant future. > Hi Jano, Apple's poll() implementation was broken in many ways over the years, but it's slowly getting better. Originally it wasn't able to deal even with stdin [1] and supported only network sockets. Also, there was a bug in macOS 10.12 [2] that was fixed in 10.12.2. And gnustep has a test for the /dev/null bug [3]. Since [3] was done 11 years ago I haven't submitted a ticket for the issue but I'm surely can do that and then I'd add a tag to commit message or a comment: Apple-Feedback: FB<ISSUE NUMBER> Would that work for you? 1. https://lists.apple.com/archives/darwin-dev/2006/Apr/msg00064.html 2. https://daniel.haxx.se/blog/2016/10/11/poll-on-mac-10-12-is-broken/ 3. https://github.com/gnustep/libs-base/blob/master/config/config.poll-dev.c Thanks, Roman
On a Wednesday in 2020, Roman Bolshakov wrote: >On Wed, Oct 07, 2020 at 07:17:05PM +0200, Ján Tomko wrote: >> On a Wednesday in 2020, Roman Bolshakov wrote: > >Hi Jano, > >> >> *indefinitely >> >Thanks, I'll correct it. > >> > BUGS >> > The poll() system call currently does not support devices. >> > >> >> Is there a bug that track this? If so, including it in a comment >> would help us delete this special case in distant future. >> > >Hi Jano, > >Apple's poll() implementation was broken in many ways over the years, >but it's slowly getting better. Originally it wasn't able to deal even >with stdin [1] and supported only network sockets. > >Also, there was a bug in macOS 10.12 [2] that was fixed in 10.12.2. And >gnustep has a test for the /dev/null bug [3]. Since [3] was done 11 >years ago I haven't submitted a ticket for the issue but I'm surely can >do that and then I'd add a tag to commit message or a comment: > >Apple-Feedback: FB<ISSUE NUMBER> > >Would that work for you? > Sounds good. Jano >1. https://lists.apple.com/archives/darwin-dev/2006/Apr/msg00064.html >2. https://daniel.haxx.se/blog/2016/10/11/poll-on-mac-10-12-is-broken/ >3. https://github.com/gnustep/libs-base/blob/master/config/config.poll-dev.c > >Thanks, >Roman >
© 2016 - 2024 Red Hat, Inc.