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

Reset 'errno' after 'sysconf()' #51

Merged
merged 1 commit into from
Jun 5, 2018
Merged

Conversation

hammond
Copy link
Contributor

@hammond hammond commented Aug 23, 2017

In our case (we haven't gotten to the bottom of exactly why), sysconf() returns -1 and sets errno == 38 (ENOSYS). When iterating over a directory, this lead to that error incorrectly being propagated after incrementing past the last directory entry.

@fxb
Copy link
Contributor

fxb commented Aug 28, 2017

To provide a bit more background on why this fix is needed:

In our case, recent Android NDKs now define _POSIX_THREAD_SAFE_FUNCTIONS, and the affected code in Boost.Filesystem now gets built. (See: android/ndk#11)

Not all platforms support it, which is why a check for _SC_THREAD_SAFE_FUNCTIONS using sysconf is correct. This will in turn cause errno to be set (See: http://man7.org/linux/man-pages/man3/sysconf.3.html). This patch resets errno to 0, since the error is handled gracefully and the code will fallback to use readdir instead of its reentrant version readdir_r.

@mic-
Copy link

mic- commented Mar 22, 2018

I can confirm that this patch fixes an issue I had where incrementing a directory_iterator would fail on a Samsung Galaxy S4 running Android 5.0.1 when building with NDK r16b.

The unpatched code works fine when built with NDK r12b, since _POSIX_THREAD_SAFE_FUNCTIONS was undefined in that NDK release. The unpatched code would also work fine on a Samsung Galaxy S7 running Android 7.0, since the _SC_THREAD_SAFE_FUNCTIONS runtime check succeeds on that device.

@alexeikh
Copy link
Contributor

I am confirming that this patch fixes bug #72 for me on Sony Xperia Z5 phone running Android 5.1.1 when building a test app with Android NDK r16b.

@alexeikh
Copy link
Contributor

alexeikh commented May 30, 2018

Non-maintainer review of the patch. Hopefully will help the maintainers to spend less time on their official review.

The patch is tiny, straightforward, very understandable. It has zero risk of introducing a regression. The patch can benefit from a code comment, explaining why errno needs to be reset 2 times. Similar to @fxb's explanation above.

Even if the patch can be improved, I think it is already good and can be merged in.

@alexeikh
Copy link
Contributor

alexeikh commented Jun 5, 2018

Dear maintainers, @Beman and @pdimov , can you please merge this tiny patch? It solves quite a major issue and has zero regression risk. Without this patch, Boost.Filesystem is basically unusable on Android. With this patch and the already-merged #69 , the next release of Boost.Filesystem can be "Android-ready".

@pdimov pdimov merged commit 5a93351 into boostorg:develop Jun 5, 2018
@rcdailey
Copy link

Which version of boost will this fix be available in? 1.67?

@pdimov
Copy link
Member

pdimov commented Jul 15, 2018

1.68, due in a few days.

@pdimov
Copy link
Member

pdimov commented Jul 15, 2018

Make that a few weeks, 1.68 is scheduled for Aug 1st. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants