search

SamuelYvon / netifaces-2

netifaces reborn
https://pypi.org/project/netifaces2/
MIT License
21 stars 7 forks source link

Rewrite Windows implementation to use GetAdaptersAddresses via Rust crate #30

Closed relativityspace-jsmith closed 3 months ago

relativityspace-jsmith commented 3 months ago

The Windows implementation in the main branch has one significant issue remaining which prevents me from using it: it does not return the loopback interface in the list of interfaces. This is a big problem for multicast_expert as it prevents opening a socket on the loopback interface!

This PR attempts to fix this issue by changing the source of interface data on Windows to the GetAdaptersAddresses() function instead of the GetAdaptersInfo(). Happily, somebody already implemented a rust crate that wraps this function in a safe wrapper, and this let me rewrite the windows code to be a lot simpler. Sadly, there was one feature I had to drop (netmasks and broadcast addresses) as it is not currently supported by the wrapper. However, I think that overall using the wrapper is the way to go as it makes the Windows version of the code way simpler and hopefully more bug immune (fingers crossed it fixes that weird memory issue that guy was seeing!).

Aside from this, I also made two other significant changes which I think should be very helpful for users.

First of all, I added a new script under examples which replicates the ip addr command using the netifaces-2 API. This should be a useful example of how to use netifaces, and it lets one easily sanity check the library on any machine by comparing the output of the script to that of ifconfig /all or ip addr.

Second of all, I changed up the situation with the AF_LINK constant. Previously, you had to pass AF_LINK on Windows to get the mac address and AF_PACKET on posix platforms. This seems like a really annoying trap for users, as it would be easy to use one constat or the other and then hey, your code is broken on Windows/Posix. To mitigate this, I simply assigned those constants to be the same value. This should work OK with legacy code while being more intuitive for users.

Hope this change looks useful, and I hope you agree that this is worth increasing the version number to 0.1.0 as the Windows implementation is now a lot cleaner!

P.S. going forward, we really need to either get in touch with the author of the get_adapters_addresses crate or fork it ourselves, because it's quite irritating that some of the functionality we need is not exposed in the wrapper. I already submitted one feature request but haven't heard back.

SamuelYvon commented 3 months ago

Second of all, I changed up the situation with the AF_LINK constant. Previously, you had to pass AF_LINK on Windows to get the mac address and AF_PACKET on posix platforms. This seems like a really annoying trap for users, as it would be easy to use one constat or the other and then hey, your code is broken on Windows/Posix. To mitigate this, I simply assigned those constants to be the same value. This should work OK with legacy code while being more intuitive for users.

Yes, these constants are the bane of this crate. Having to keep compat. with the old library is a bit of a pain since like you correctly identified the values are platform dependents.

Hope this change looks useful, and I hope you agree that this is worth increasing the version number to 0.1.0 as the Windows implementation is now a lot cleaner!

I'm still hesitant, but you seem to want to include it in your crate, so I assume it's safe enough for you ;)

SamuelYvon commented 3 months ago

Actually I'll just merge right away, I'll re-add the error when it's actually useful.

SamuelYvon commented 3 months ago

Thanks for the hard work!