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

Fix raw_fallocate for Android and deal with unsupported filesystems. #2363

Merged

Conversation

de-vri-es
Copy link
Contributor

@de-vri-es de-vri-es commented Jun 28, 2018

This PR does two things:

It's quite important not to fall back to lseek+write on all allocation failures, since the whole reason for the allocation was to prevent SIGBUS errors when allocation fails after mmaping. Ignoring allocation failure for supported filesystems would defeat that purpose, but always failing on unsupported filesystems even if space is available does not seem acceptable.

I skipped the fallback to ftruncate, because I think it is equally likely to fail due to lack of filesystem support if (posix_)fallocate did.

I also didn't implement a fallback in the OS X version because I can't find any relevant documentation on the failure modes. It could be useful if someone on OS X tries to call raw_fallocate on a file on FAT32 and NTFS filesystem to see if that fails, and with what errno value.

@taketwo
Copy link
Member

taketwo commented Jun 29, 2018

LGTM, thanks! It would be nice to hear from an Android user before merging this, so let's wait some time.

@taketwo taketwo added needs: testing Specify why not closed/merged yet module: io labels Jun 29, 2018
@taketwo
Copy link
Member

taketwo commented Aug 26, 2018

@SergioRAgostinho I think we should also merge this and hope for the best.

@SergioRAgostinho SergioRAgostinho removed the needs: testing Specify why not closed/merged yet label Aug 26, 2018
@SergioRAgostinho SergioRAgostinho merged commit 5b89b76 into PointCloudLibrary:master Aug 26, 2018
@shelper
Copy link

shelper commented May 17, 2020

is it for sure the issue is fixed for android?
I failed because of this issue, see:
#4107

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

Successfully merging this pull request may close these issues.

4 participants