Upgrade to winapi 0.3 #363
Conversation
Pull Updates from fork
|
Hmm, tests are not passing on appveyor, but they are passing locally. Getting my env setup for older rust now and will recheck. Finally got my tests passing after I (RTFM) ran in powershell, $env:RUST_TEST_THREADS="1" then re-ran the same cargo test. |
|
All tests should now be passing, trying to fix git history that got corrupted between linux/win machines, more rookie mistakes. |
|
@mrmonday Would you have any time today/tomorrow to chat about this PR and some questions in IRC? I think I understand how to do all tests and get them to pass now, also found an OSX/BSD issue and addressed it. |
|
Awesome! Have been waiting for this. |
|
@Draphar glad to hear it. Hopefully someone using or contributing to this project can help me with a second code review. This is my first real open source contribution. I've likely made mistakes, possibly many lol. |
| @@ -0,0 +1,7 @@ | |||
| # rls.toml | |||
There was a problem hiding this comment.
I haven't seen an rls.toml before - would you mind sharing a link to some docs where I can read more?
There was a problem hiding this comment.
Oh no! My apologies. Shouldn't have let that get committed. Will fix! Also, this is how I have my atom extension setup https://atom.io/packages/ide-rust , when I am using Atom. Seems most ides have issues with rls and complex projects, variable rust versions. So I switch as required, they all seem to be constantly broken in some way.l
| @@ -96,88 +90,3 @@ fn htons(u: u16) -> u16 { | |||
| fn ntohs(u: u16) -> u16 { | |||
| u16::from_be(u) | |||
| } | |||
|
|
|||
| fn make_in6_addr(segments: [u16; 8]) -> In6Addr { | |||
There was a problem hiding this comment.
How come these have been moved?
There was a problem hiding this comment.
These were split up and moved as libc::in6_addr is a different struct with different fields/nested-fields than winapi-rs 0.3. winapi-rs 0.3 is more explicit with its handling of unions so instead of just calling it a DWORD for example, it calls it a struct with a field that is named Word, and accessed via a method. These methods are marked as unsafe. See here: https://docs.rs/winapi/0.3.6/winapi/shared/in6addr/struct.in6_addr_u.html. I figured splitting them into OS specific files made more sense, feel free to correct me.
| winsock2::setsockopt(socket, level, name, value, option_len) | ||
| } | ||
|
|
||
| pub fn addr_to_sockaddr(addr: SocketAddr, |
There was a problem hiding this comment.
Hmm - were these duplicated when you moved them? Is there a reason to have them multiple times rather than just once?
There was a problem hiding this comment.
There was a reason, can't say for certain it was a "good" reason. See above comment reply.
| (addr.S_un as u32).to_be() | ||
| unsafe { | ||
| (*addr.S_un.S_addr() as u32).to_be() | ||
| } |
There was a problem hiding this comment.
This wasn't unsafe before - why has this changed?
There was a problem hiding this comment.
See comment above. the S_addr() method for the struct field is generated via a new macro in winapi 3, as this is supposed to be a union in future rust versions. So this method is actually unsafe, in future rust versions, I believe this will be unnecessary, I think, due to native union support.
| let mut val = InAddr::default(); | ||
| unsafe { | ||
| *val.S_un.S_addr_mut() = addr as minwindef::ULONG; | ||
| } |
There was a problem hiding this comment.
Same question as above - how come this is unsafe now?
There was a problem hiding this comment.
See above comment re: struct field access method.
| htons(segments[7])]; | ||
| } | ||
|
|
||
| // val = unsafe { |
There was a problem hiding this comment.
Commented code can be removed :)
There was a problem hiding this comment.
I left it in there, and forgot to comment it with a question. The original code uses mem::transmute(). I'm pretty new to Rust, so I won't presume I know better than others. It made me wonder, based on rust docs and guides, if mem::transmute can be removed entirely from libpnet, same with mem::uninitialized. I'd like to know more about the use cases for that, when its preferred and why. My code replacement is still unsafe, due to the Word_mut() method being unsafe, but it seems safer than transmute, right?
|
I think this looks good overall - I've left a few comments. I won't merge this until the upstream changes have made their way into I'm a bit busy at the moment - if you're able to idle on IRC you can ask questions and I'll respond when I can. Re: Minimum supported version - this can be bumped as high as it needs to go, I don't have a problem with that. Just needs the CI and docs updating to mention it. Re: 2018 - I'm happy to move libpnet to it, but it should be in a separate PR. |
|
@mrmonday No rush, I can be patient (overly eager lol). I will address each of your comments, but also ask some questions, too. I can indeed idle on IRC. RE: version and 2018, I wasn't even thinking about the fact that cargo essentially snapshots versions, so we can move the github project forward, while not breaking existing deployed code. I will create an issue to move to 2018 and bump versions. Will do a PR for it once ready. |
|
@mrmonday Also, since I'm still learning on merges and PRs. Would we be able to squash merge this PR when moved to master, to erase my commit message and merge history mistakes? |
|
You can either do it yourself, or I can do it when I merge it. |
|
Whats the status on this PR? |
|
@Shock-1 -- Unfortuantely I've been out of the loop on this one so I'm not sure what the status is. There are quite a number of conflicts raised from this now |
|
I think we should move on to apply this effort. Let me try to resolve conflict first. |
|
prepared PR #438 for current main trunk patch. |
|
is there anybody who can try winapi 0.3 version from #438? I tried a little but not confident this works fine on actual Windows environment... |
My usual rule is "if it passes CI, it's good". If it turns out not to be, CI needs improving - either to add missing test cases, or to test in the problematic environment. |
|
@mrmonday thanks, your comment encourage me to go forward. Let me clean up code and incorporate the change. |
|
Closing this, looks like we're using winapi 0.3 now. |
This PR is to move away from legacy winapi 0.2 and related *-sys crates to winapi 0.3.x. Note, this is currently using my git branch iphlpapi as required changes have not yet been merged into upstream, yet.
This is my second PR ever, and I'm pretty green with github, merges, forks, etc.., so I do apologize for rookie mistakes in advance and welcome any and all commentary/criticism.
This is a WIP PR, I updated to winapi 0.3 naively using rust-2018 and nightly. I need to go back and support this project's minimum Rust version. winapi 0.3 supports back to rust 1.6 so there should be no issues there.
Tests are not yet passing, for me.