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

sys/socket: add UnixAddr abstract name getter #785

Merged
merged 1 commit into from
Nov 19, 2017

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Oct 26, 2017

This introduces an as_abstract() getter to UnixAddr in order to
retrieve the name of an abstract unix socket.
This also adds tests around abstract addresses and clarify docs,
adding explicit semantics.

@lucab
Copy link
Contributor Author

lucab commented Oct 26, 2017

It looks like master is currently broken due to this libc change: rust-lang/libc#787.
/cc @vojtechkral

I also realized I forgot the relevant CHANGELOG entry, I'll add it on the next rebase once master is fixed. Reviews and comments still welcome in the meanwhile.

@vojtechkral
Copy link

Hi @lucab
To fix this, you probably just need to change #[repr(i32)] of the EventFilter to #[repr(u32)]. (OTOH if you believe my fix was incorrect or should not have been merged, please feel free to comment in the libc issue thread.)

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Even though new_abstract and as_abstract compile fine everywhere, they won't work except on Linux. So they should probably be guarded with a #[cfg(target_os = "linux")]

@lucab
Copy link
Contributor Author

lucab commented Nov 6, 2017

PTAL on second iteration:

  • rebased on a green master
  • added CHANGELOG entries
  • restricted abstract addresses methods to Linux only

@asomers I did as you suggested but I am not a fan of it, because UnixAddr is just a container for a typed address. I don't see why it should conditionally change depending on where you build. Perhaps a LinuxExt trait?

@asomers
Copy link
Member

asomers commented Nov 11, 2017

@lucab The #[cfg(target_os = "linux")] guard doesn't care about where you build, only where you target. And abstract sockets only work on Linux*. So it doesn't make sense to enable that code for any other OS. And I don't see what value a LinuxExt trait would add. Could you please explain?

  • It would be worth checking if Android, which is considered a separate OS by Rust, supports them too.

This introduces an `as_abstract()` getter to `UnixAddr` in order to
retrieve the name of an abstract unix socket.
This also adds tests around abstract addresses and clarify docs,
adding explicit semantics.
@lucab
Copy link
Contributor Author

lucab commented Nov 19, 2017

@asomers I'm fine with your feedback, I don't want to keep bikeshedding this further.

I've double-checked Android docs and then added support for it as well, PTAL.

@asomers
Copy link
Member

asomers commented Nov 19, 2017

Thanks for your submission.

bors r+

bors bot added a commit that referenced this pull request Nov 19, 2017
785: sys/socket: add UnixAddr abstract name getter r=asomers a=lucab

This introduces an `as_abstract()` getter to `UnixAddr` in order to
retrieve the name of an abstract unix socket.
This also adds tests around abstract addresses and clarify docs,
adding explicit semantics.
@bors
Copy link
Contributor

bors bot commented Nov 19, 2017

@bors bors bot merged commit 02f2ede into nix-rust:master Nov 19, 2017
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

3 participants