Skip to content

Conversation

@Therzok
Copy link
Contributor

@Therzok Therzok commented Feb 24, 2021

Fixes #47584

@ghost ghost added the area-PAL-libraries label Feb 24, 2021
@marek-safar marek-safar requested a review from janvorli February 24, 2021 09:31
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Might be worth adding a comment to the effect that EINTR isn't documented but has been shown to happen in practice. Otherwise, LGTM. Thanks for the fix.

@Therzok
Copy link
Contributor Author

Therzok commented Feb 24, 2021

Added comments. I'm pretty sure readdir_r is being used on macOS, so I didn't guard readdir too.

int error;

// EINTR isn't documented, happens in practice on macOS.
while ((error = readdir_r(dir, entry, &result)) && error == EINTR);
Copy link
Member

Choose a reason for hiding this comment

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

are you sure the EINTR is returned as the return value and not in errno?

Copy link
Contributor Author

@Therzok Therzok Feb 25, 2021

Choose a reason for hiding this comment

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

I don't have a reliable repro, but I can look in xnu source. I can adapt it so it checks errno.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like readdir_r sets the return value to whatever is in errno?

Copy link
Member

Choose a reason for hiding this comment

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

I've read a few man pages of different OSes and there seems to be little consistency regarding this.
If using errno works on macOS then I'd prefer that since it's what we use everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it.

Copy link
Member

Choose a reason for hiding this comment

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

If using errno works on macOS then I'd prefer that since it's what we use everywhere else.

Except this API explicitly says errors are returned rather than surfacing through errno. Is there an example man page that says readdir_r sets errno? What happens if a readdir_r implementation doesn't touch errno at all, errno retains an EINTR value from prior to the readdir_r call, and readdir_r hits another error... might we end up in an infinite loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the code back to checking the return code :D

Copy link
Member

Choose a reason for hiding this comment

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

Before you do, I'd like to hear from @akoeplinger :) I was actually asking questions rather than stating things that must happen ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ghost
Copy link

ghost commented Feb 25, 2021

Hello @akoeplinger!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@akoeplinger
Copy link
Member

Test failures are due to #48751.

@akoeplinger akoeplinger merged commit 5a561eb into dotnet:master Feb 25, 2021
@Therzok Therzok deleted the macos-eintr branch February 25, 2021 20:19
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing EINTR handling on macOS for opendir/readdir/closedir syscalls

4 participants