Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

select: align poll emulation to return all relevant events #5883

Closed
wants to merge 1 commit into from
Closed

select: align poll emulation to return all relevant events #5883

wants to merge 1 commit into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Aug 28, 2020

The poll emulation via select already consumes POLLRDNORM,
POLLWRNORM and POLLRDBAND as input events. Therefore it
should also return them as output events if signaled.

Also fix indentation in input event handling block.

Assisted-by: Daniel Stenberg

Replaces #5852

@mback2k mback2k requested a review from bagder August 28, 2020 18:44
@mback2k mback2k self-assigned this Aug 28, 2020
@mback2k mback2k mentioned this pull request Aug 28, 2020
Closed
@mback2k mback2k changed the title select: fix poll emulation to return all relevant events select: align poll emulation to return all relevant events Aug 28, 2020
@mback2k mback2k force-pushed the select-fix-poll-emulation branch 2 times, most recently from 0b0adb5 to 9d093fd Compare August 29, 2020 08:14
@mback2k mback2k requested a review from jay August 30, 2020 10:24
jay
jay requested changes Aug 31, 2020
View reviewed changes
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is right. For example you are always setting POLLRDNORM on POLLIN afterwards but afaics that flag was not necessarily set beforehand. My understanding of poll is it is only supposed to write bits that were passed in, with the exceptions noted below. I think I agree with @bagder maybe it's best to take a if it ain't broke don't fix it approach here.

from posix poll:
In each pollfd structure, poll() shall clear the revents member, except that where the application requested a report on a condition by setting one of the bits of events listed above, poll() shall set the corresponding bit in revents if the requested condition is true. In addition, poll() shall set the POLLHUP, POLLERR, and POLLNVAL flag in revents if the condition is true, even if the application did not set the corresponding bit in events.

@mback2k
Copy link
Member Author

mback2k commented Aug 31, 2020

I don't know if this is right. For example you are always setting POLLRDNORM on POLLIN afterwards but afaics that flag was not necessarily set beforehand. My understanding of poll is it is only supposed to write bits that were passed in, with the exceptions noted below.

I don't think this is something that I am changing with this PR. The current code also sets POLLIN if that itself was not set, but instead POLLRDNORM was set. So I am basically extending it to be also the case the other way around. Of course, I can extend this PR to make sure to check for both flags being requested before setting them. Please see how the code already handles both types of flags on the pre-poll code path which I just want to reflect in the post-poll code path as well:

curl/lib/select.c

Lines 400 to 405 in 147eca8

if(ufds[i].events & (POLLRDNORM|POLLIN))
FD_SET(ufds[i].fd, &fds_read);
if(ufds[i].events & (POLLWRNORM|POLLOUT))
FD_SET(ufds[i].fd, &fds_write);
if(ufds[i].events & (POLLRDBAND|POLLPRI))
FD_SET(ufds[i].fd, &fds_err);

I think I agree with @bagder maybe it's best to take a if it ain't broke don't fix it approach here.

I somewhat agree on this, but I think the current code is partially broken, because it is not following the spec you referenced either. I will update this PR to reflect the spec even better.

@mback2k
select: align poll emulation to return all relevant events
b517f72 
The poll emulation via select already consumes POLLRDNORM,
POLLWRNORM and POLLRDBAND as input events. Therefore it
should also return them as output events if signaled.

Also fix indentation in input event handling block.

Assisted-by: Daniel Stenberg
Assisted-by: Jay Satiro

Replaces curl#5852
Closes curl#5883
@mback2k mback2k requested a review from jay August 31, 2020 08:50
@mback2k
Copy link
Member Author

mback2k commented Aug 31, 2020

I updated the PR to be in line with the POSIX specification. This should also make sure there are no side effects, because we won't return the "new" flags POLLRDNORM, POLLWRNORM and POLLRDBAND if they were not requested by the caller beforehand.

@jay
Copy link
Member

jay commented Aug 31, 2020

Fair enough, looks better now.

@mback2k
Copy link
Member Author

mback2k commented Sep 1, 2020

@jay thank you very much! @bagder what do you think of this new variant?

@mback2k mback2k closed this in 20d74b7 Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

@jay jay Awaiting requested review from jay

Assignees

@mback2k mback2k

Labels
tidy-up
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mback2k @jay @bagder