[PATCH] tests: commandhelper: Accept POLLNVAL on macOS
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
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Roman Bolshakov