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

Implement nic on windows #297

Merged
merged 8 commits into from Feb 28, 2021
Merged

Conversation

daladim
Copy link
Contributor

@daladim daladim commented Dec 18, 2020

Hello, I had to implement a piece of heim for my needs, let"s share it!

This MR implements heim::net::nic() on Windows using the GetAdaptersAddresses API.
It closes #24

It also adds an index() member function for Nics on Windows, macOS and Linux

@coveralls
Copy link

coveralls commented Dec 18, 2020

Coverage Status

Coverage increased (+3.8%) to 38.279% when pulling aa790c7 on daladim:implement_nic_on_windows into c111eb4 on heim-rs:master.

@svartalf
Copy link
Member

Hey, @daladim, amazing work!

There is a lot to review and due to holiday season I'm not sure if I'll be able do that very soon, but your change looks very helpful and promising, so I'll try my best to do that asap

@daladim
Copy link
Contributor Author

daladim commented Feb 11, 2021

Hi @svartalf , did you have time to review these changes? Do you want any help to do so?

@svartalf
Copy link
Member

@daladim I'm sorry, definitely not cool from my side.
I started reviewing it a few days ago and will try to finish soon; so far everything looks great, but you should be very careful with unsafe after all :)

Copy link
Member

@svartalf svartalf left a comment

Choose a reason for hiding this comment

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

Looks good in general, I have one tiny nitpick regarding the mem::size_of usage and after that I'll merge this PR

heim-net/src/sys/windows/nic.rs Outdated Show resolved Hide resolved
}

pub fn is_running(&self) -> bool {
unimplemented!()
// TODO: not sure how to tell on Windows
true
Copy link
Member

Choose a reason for hiding this comment

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

As a reminder: after all reviews it would be nice to create tracking issues for this and above TODOs in order not to forget about these things.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://todo.jasonet.co/ I've used this in some projects to great effect. It's a bot that automatically opens issues for TODO comments when merging a PR. Thought you might be interested ^^.

@daladim
Copy link
Contributor Author

daladim commented Feb 12, 2021

@daladim I'm sorry, definitely not cool from my side.

Nothing to hurry about, I was just checking all my pending pull requests. Thanks for the review!

Copy link
Member

@svartalf svartalf left a comment

Choose a reason for hiding this comment

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

I've also found GetIfTable2 and MIB_IF_ROW2, which is also used by psutil (except they are using outdated GetIfTable).

Looks like it also have a lot of interesting information, @daladim, I'm curious if you checked it before? GetInterfaceAddresses covers current public API, but this thing is also might be interesting.

Unfortunately, I can't find how to provide information for destination and is_running methods, which means that if we will not be able to figure them out, I'll demote them to be *nix-specific methods and move into heim::net::os::nix::NicExt (it is out of scope for this PR, though).

heim-net/src/sys/windows/nic.rs Outdated Show resolved Hide resolved
@daladim
Copy link
Contributor Author

daladim commented Feb 22, 2021

@daladim, I'm curious if you checked it before?

There are indeed quite a few Microsoft APIs to retrieve info about network interfaces. I've had a look at all of them (at least all which are part of the IP Helper API), but it turns out GetAdaptersAddresses is the only one that supports both loopback interfaces and IPv6, so that's the one I've used.

@daladim
Copy link
Contributor Author

daladim commented Feb 22, 2021

As to the merge conflicts, do you prefer a rebase on master, or a merge commit that merges master on my current branch?

@svartalf
Copy link
Member

but it turns out GetAdaptersAddresses is the only one that supports both loopback interfaces and IPv6, so that's the one I've used.

Thanks, looks like the best choice we have :)

As to the merge conflicts, do you prefer a rebase on master, or a merge commit that merges master on my current branch?

No personal choice from me, I usually just squashing branches before merge into master branch :|

@daladim
Copy link
Contributor Author

daladim commented Feb 25, 2021

I've rebased on master to solve the merge conflicts (basically due to the + Sync + Send that you added on the trait while I edited the lines around these signatures)

@svartalf svartalf merged commit ad69138 into heim-rs:master Feb 28, 2021
@svartalf
Copy link
Member

Some new clippy lint breaks CI, but I'll fix that separately on my own.
Alright, this is huge addition and a tremendeous amount of work, thanks you so much, @daladim!

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.

Network NIC
4 participants