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

Upgrade to winapi 0.3 #363

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mattlknight
Copy link

mattlknight commented Mar 5, 2019

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.

mattlknight added some commits Aug 27, 2018

Merge pull request #1 from libpnet/master
Pull Updates from fork
@mattlknight

This comment has been minimized.

Copy link
Author

mattlknight commented Mar 5, 2019

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.

@mattlknight mattlknight force-pushed the mattlknight:winapi3 branch 4 times, most recently from 766568e to 5708469 Mar 5, 2019

@mattlknight

This comment has been minimized.

Copy link
Author

mattlknight commented Mar 5, 2019

All tests should now be passing, trying to fix git history that got corrupted between linux/win machines, more rookie mistakes.

@mattlknight mattlknight force-pushed the mattlknight:winapi3 branch from 5708469 to 3ca2c0b Mar 5, 2019

@mattlknight mattlknight force-pushed the mattlknight:winapi3 branch from 73c7502 to 8743a31 Mar 5, 2019

@mattlknight

This comment has been minimized.

Copy link
Author

mattlknight commented Mar 5, 2019

@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.

@Draphar

This comment has been minimized.

Copy link
Contributor

Draphar commented Mar 6, 2019

Awesome! Have been waiting for this.

@mattlknight

This comment has been minimized.

Copy link
Author

mattlknight commented Mar 6, 2019

@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

This comment has been minimized.

@mrmonday

mrmonday Mar 6, 2019

Contributor

I haven't seen an rls.toml before - would you mind sharing a link to some docs where I can read more?

This comment has been minimized.

@mattlknight

mattlknight Mar 6, 2019

Author

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 {

This comment has been minimized.

@mrmonday

mrmonday Mar 6, 2019

Contributor

How come these have been moved?

This comment has been minimized.

@mattlknight

mattlknight Mar 6, 2019

Author

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,

This comment has been minimized.

@mrmonday

mrmonday Mar 6, 2019

Contributor

Hmm - were these duplicated when you moved them? Is there a reason to have them multiple times rather than just once?

This comment has been minimized.

@mattlknight

mattlknight Mar 6, 2019

Author

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()
}

This comment has been minimized.

@mrmonday

mrmonday Mar 6, 2019

Contributor

This wasn't unsafe before - why has this changed?

This comment has been minimized.

@mattlknight

mattlknight Mar 6, 2019

Author

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;
}

This comment has been minimized.

@mrmonday

mrmonday Mar 6, 2019

Contributor

Same question as above - how come this is unsafe now?

This comment has been minimized.

@mattlknight

mattlknight Mar 6, 2019

Author

See above comment re: struct field access method.

htons(segments[7])];
}

// val = unsafe {

This comment has been minimized.

@mrmonday

mrmonday Mar 6, 2019

Contributor

Commented code can be removed :)

This comment has been minimized.

@mattlknight

mattlknight Mar 6, 2019

Author

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?

@mrmonday

This comment has been minimized.

Copy link
Contributor

mrmonday commented Mar 6, 2019

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 winapi - I would rather depend on the official version of the crate.

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.

@mattlknight

This comment has been minimized.

Copy link
Author

mattlknight commented Mar 6, 2019

@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.

@mattlknight

This comment has been minimized.

Copy link
Author

mattlknight commented Mar 6, 2019

@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?

@mrmonday

This comment has been minimized.

Copy link
Contributor

mrmonday commented Mar 6, 2019

You can either do it yourself, or I can do it when I merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.