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

Add ENOTSUP to Linux and Android #969

Merged
merged 1 commit into from
Mar 29, 2019
Merged

Add ENOTSUP to Linux and Android #969

merged 1 commit into from
Mar 29, 2019

Conversation

jmmv
Copy link
Contributor

@jmmv jmmv commented Nov 8, 2018

While ENOTSUP is defined as equal to EOPNOTSUPP on these platforms,
exposing the ENOTSUP symbol (as libc does) allows for writing portable
code that may want to reference this error code.

@jmmv
Copy link
Contributor Author

jmmv commented Nov 8, 2018

I have no idea if this will work. Created the PR to trigger a CI build and seeing what happens.

@asomers
Copy link
Member

asomers commented Nov 9, 2018

I'm not sure about this PR. What portability advantages does ENOTSUP have ove EOPNOTSUP?

@jmmv
Copy link
Contributor Author

jmmv commented Nov 9, 2018

I had some code that returned Errno::ENOTSUP. This built and worked fine on macOS, but when I tried the same code on Linux, it failed because Linux didn't expose this symbol. Therefore, it is not possible to have such code today while keeping it portable.

(I changed my code to EOPNOTSUPP instead because I don't really care about the value, and this works on both macOS and Linux. But the issue remains and I guess it could confuse someone else in the future or prevent some form of code that truly needs to use ENOTSUP.)

@jmmv
Copy link
Contributor Author

jmmv commented Mar 8, 2019

Just getting back to this because of bazelbuild/sandboxfs#72. Note how my "fix" to use EOPNOTSUPP to support Linux and macOS was not good: this symbols doesn't exist in FreeBSD and the reporter suggested switching to ENOTSUP instead because it would work on other systems (but it wouldn't). I think we should expose both symbols all the time.

@asomers
Copy link
Member

asomers commented Mar 8, 2019

Ok, but let's not duplicate libc's job. Instead of defining ENOTSUP as an alias for EOPNOTSUP we should just expose whatever libc exposes. As you wrote it, the build will fail on NetBSD because libc doesn't expose EOPNOTSUP on that platform. Things are even worse on OSX and Newlib, where EOPNOTSUP is distinct from ENOTSUP.

While ENOTSUP is defined as equal to EOPNOTSUPP on these platforms,
exposing the ENOTSUP symbol (as libc does) allows for writing portable
code that may want to reference this error code.
@jmmv
Copy link
Contributor Author

jmmv commented Mar 29, 2019

OK, so I'm not sure how you'd like to achieve this.

First, the changes I made are all within a chunk of code that is gated by a #cfg for Linux and Android only, so I cannot see how they could break NetBSD nor macOS per your last comment.

Second, I tried adding ENOTSUP = libc::ENOTSUP within the Errno enum for Linux, but that doesn't work because we end up with two different Errno entries that map to the same numeric value.

And lastly, note that the line I'm adding exists next to definitions for EWOULDBLOCK and EDEADLOCK outside of the enum that explicitly map to other values (thus theoretically duplicating logic within libc). I'm guessing they do this precisely for the same reason I'm trying to expose ENOTSUP. So copying whatever is there already seems sensible.

@asomers
Copy link
Member

asomers commented Mar 29, 2019

I didn't realize that Rust enforced a uniqueness constraint for enums. Ok, let's commit this then.

bors r+

bors bot added a commit that referenced this pull request Mar 29, 2019
969: Add ENOTSUP to Linux and Android r=asomers a=jmmv

While ENOTSUP is defined as equal to EOPNOTSUPP on these platforms,
exposing the ENOTSUP symbol (as libc does) allows for writing portable
code that may want to reference this error code.

Co-authored-by: Julio Merino <jmmv@google.com>
@bors
Copy link
Contributor

bors bot commented Mar 29, 2019

Build succeeded

@bors bors bot merged commit d068383 into nix-rust:master Mar 29, 2019
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.

None yet

2 participants