Implement extra traits for all types #1035
Conversation
|
Would it be possible to split out just the |
|
Now that rust-lang/libc#1371 was merged, it's time to come back to this. That should have resolved most of the issues. Gonna test this through CI then do another pass through @asomers' comments. |
|
Sorry I wasn't clear. I was planning to write new tests. The problem is that I dont know how to write them. I can handcraft sockaddr_un structs to test equality, but that seems like I'm coding the same logic in the test and I'm testing for and I dont think it'll provide much value. Or I can modify the stack so that all values are nonzero and use UnixAddr directly to do the comparison. I think the latter would be a better implementation, but I don't know how to do it.
On Wed, Jun 5, 2019, at 8:44 AM, Alan Somers wrote:
***@***.**** commented on this pull request.
In src/sys/socket/addr.rs <#1035 (comment)>:
> @@ -687,27 +595,6 @@ impl UnixAddr {
}
}
…-impl PartialEq for UnixAddr {
- fn eq(&self, other: &UnixAddr) -> bool {
- self.sun_path() == other.sun_path()
>> Yeah, we should definitely be testing equality for abstract sockets. I would suggest just manually creating some so we can get all of the edge cases.
> Turns out we have some tests that check the paths for regular and abstract sockets. I was thinking of extending them, but I'm not certain the right way to do it. I was going to create a `u8` array of 0xFF the same size of the struct, transmute it to a `UnixAddr` and then manually set the `sockaddr_un` and `usize` fields. This will have a bunch of custom logic to set those fields the way I think they should be set.
> What I would prefer to do is to initialize a `UnixAddr`, then set the stack to contain `0xFF` for a large chunk of memory, and initialize an identical `UnixAddr`, and then compare them for equality and hashing. That should test it very effectively and remove all of the complexity of stuffing data in correctly by hand. Do you know of a way to do that with Rust?
That sounds cumbersome. Instead of modifying those tests, why not write new tests directly in src/sys/socket/addr.rs? That way you'll be able to access the underlying libc::sockaddr_un directly.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#1035?email_source=notifications&email_token=AABSMG6FAYL5OK7OTNJEAMLPY7NMTA5CNFSM4G45KN3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB2VR72Q#discussion_r290808764>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABSMG45V2F4H7YQCNPJJUTPY7NMTANCNFSM4G45KN3A>.
|
I think the most straightforward way would be to handcraft a |
|
Alright, I implemented something that I think is a reasonable test. Tests passed on my local computer, so if CI passed, I think this is good to go. |
Susurrus
force-pushed
the
extra_traits
branch
2 times, most recently
from
932a219
to
af31e92
Compare
|
Looks like |
Yeah, that one stumped me too. I installed the exact mips64 target for the 1.24.1 toolchain and checked that I'm building against the exact same revision of libc as Travis used, but for me it works. |
|
Figured out I wasn't synced to HEAD and 4bd419ebec2d691f55331e91e799d5fa96180044 was the problem. Filed rust-lang/libc#1393 to resolve. |
Re-add PTRACE_DETACH for mips64 GNU targets During the refactor in 4bd419e, `PTRACE_DETACH` was accidentally removed on these targets. This re-adds it. Fixes brokenness encountered during nix-rust/nix#1035 CC @asomers
|
Alright, looks like we should be all good to go here now that my |
Also bump Rust requirement to 1.25 which is a requirement of that feature
Derive Clone, Copy, Eq, Hash, and PartialEq for all types. Not all traits are supported by all types, which is why many are missing some.
As this is now derived, having a test is unnecessary
Switch to manual trait impls for sigevent `sigevent` on most platforms have padding or unused fields. Rather than display those in the `Debug` impl by deriving it, manually implement all `extra_traits` instead ignoring those fields. I do worry that my `PartialEq` implementations for this for some platforms (like Linux) is not correct due to ignoring bytes that shouldn't be ignored because these structs don't have a proper union set up. cc @asomers Part of nix-rust/nix#1035
|
Alright, I think we're all good with this one. Libc changes have all been merged and tests are all padsing |
Abstract paths should always be N-1 in length where N is the length of the `sun_path` field (first byte is \0). Given that, `UnixAddr::new_abstract()` should always return this N-1 length, not just the length of the string provided (the rest of the array will be \0s).
This is a minor optimization that allows for reduced sizes of datatypes. Since this pointer will never be NULL, it's safe to use here
|
Hopefully this is the final necessary batch of changes! |
1035: Implement extra traits for all types r=asomers a=Susurrus Now that I've gotten all the extra traits implemented for `libc`, they can be easily derived for `nix`'s types. Note that this requires a bump to the minimum supported Rust version, but it's still at least 2 versions behind, so it fits in with policy. One thing I did notice is that we have an inconsistent approach to our newtypes, where some use a struct and some a tuple struct. We should be consistent here, and should probably use a tuple struct since the name of the single field is irrelevant. This style is already suggested in our `CONVENTIONS.md` doc, so this should be uncontroversial. I'll file a PR after this is merged adding that. Co-authored-by: Bryant Mairs <bryant@mai.rs>
Build succeeded
|
Now that I've gotten all the extra traits implemented for
libc, they can be easily derived fornix's types. Note that this requires a bump to the minimum supported Rust version, but it's still at least 2 versions behind, so it fits in with policy.One thing I did notice is that we have an inconsistent approach to our newtypes, where some use a struct and some a tuple struct. We should be consistent here, and should probably use a tuple struct since the name of the single field is irrelevant. This style is already suggested in our
CONVENTIONS.mddoc, so this should be uncontroversial. I'll file a PR after this is merged adding that.