Added 'get_mac_address_by_ip' which does what it says #42
Conversation
Also added ./example/lookup_by_ip.rs which correctly outputs the MacAddress on my windows machine. Linux implementation in the next commit.
Also added ./example/lookup_by_ip.rs which correctly outputs the MacAddress on my windows machine. Linux implementation in the next commit.
capveg-netdebug
force-pushed
the
01-11-Added_get_mac_address_by_ip_which_does_what_it_says
branch
from
7fb63b0 to
3833e41
Compare
getaddrif() does not return the information indexed by adapter so we have to build our own index. Tested on my Linux vm - seems to work!
| let ifiter = getifaddrs()?; | ||
|
|
||
| let mut ip_on_inferface: Option<String> = None; | ||
| let mut mac_to_interface: HashMap<String, Option<[u8; 6]>> = HashMap::new(); |
There was a problem hiding this comment.
I'm a little confused what the purpose of this HashMap is, if we already have the interface IP and MAC while we're iterating over them
There was a problem hiding this comment.
So, as I understand it, the getifaddrs()'s call returns all of the addresses (MAC, IPv4, IPv6, etc.) of all of the interfaces on the system in a flat structure, e.g., there's no single record that has both the MAC and the IP address. So there will (ideally) be one record with the target IP and a different record with the resulting MAC address. The HashMap is just creating an index/map from each interface to its MAC address so, for example, if we find the MAC address entry first, we record it and then if we find the IP entry later, we can look it up and return it. Similarly, because the order of the records that getifaddrs() returns isn't guaranteed, we could in theory find the IP address we're looking for first, and then the MAC address in a later record, so we have to handle that case as well.
Does that make sense? Did I understand it correctly? FYI: I did look in the nix crate for a more direct function to do the mapping but couldn't find it, so there may be a better way to do this.
There was a problem hiding this comment.
Thanks for the reviews! FYI: I ran this on both a Linux and Windows system and "it worked for me" but it's wasn't clear how to do deeper testing for this type of tightly coupled system calls. Let me know about the rest and I'll do another pass on the code.
There was a problem hiding this comment.
Hmm, I see. Thanks for the info, I'll do some further investigating and see what I can find wrt. that specific bit of code 🤔
| if let Some(adapter_ip) = if let Some(sin4) = iface_address.as_sockaddr_in() { | ||
| // v4 addr? | ||
| Some(IpAddr::from(Ipv4Addr::from(sin4.ip()))) | ||
| } else if let Some(sin6) = iface_address.as_sockaddr_in6() { | ||
| // v6 addr? | ||
| Some(IpAddr::from(Ipv6Addr::from(sin6.ip()))) | ||
| } else { | ||
| // something else, ignore | ||
| None | ||
| } { | ||
| // found an IP for this adapter - if it's the one we're looking for, save it | ||
| if adapter_ip == *ip { | ||
| ip_on_inferface = Some(interface.interface_name.clone()); | ||
| if let Some(mac) = mac_to_interface.get(&interface.interface_name) { | ||
| return Ok(mac.clone()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This section is pretty confusing, using if let ...s for the condition of another if let directly is also pretty unidiomatic, so I think it'd be better if you bound the IP to a variable first, and continue on None instead.
There was a problem hiding this comment.
I disagree about it being un-idiomatic - this is fairly standard functional programming which Rust explicitly tries to support (https://doc.rust-lang.org/book/ch13-00-functional-features.html). That said, it's your code and I'd rather you agree to land it than argue about it so I can change it. Let me know if the my response to your first comment makes sense and then I can tackle any changes together with this.
There was a problem hiding this comment.
I wouldn't call that style functional, it's an if ... else if ... else that evaluates to a bool, but regardless, my concern is it took me about ~5 minutes to figure out how that was compiling until I finally broke it down (and I work with Rust almost every day and have seen it before 😄), its just pretty hard to read IMO with the condition being at the same level as the actual following block, and } { being pretty uncommon, and would better be served by a .map or .and_then on another binding (or even as the conditional).
| let adapter_ip = match unsafe { (*sock_addr).sa_family } as i32 { | ||
| AF_INET => unsafe { | ||
| let addr = (*(sock_addr as *mut SOCKADDR_IN)).sin_addr; | ||
| Some(IpAddr::from([ | ||
| addr.S_un.S_un_b().s_b1, | ||
| addr.S_un.S_un_b().s_b2, | ||
| addr.S_un.S_un_b().s_b3, | ||
| addr.S_un.S_un_b().s_b4, | ||
| ])) | ||
| }, | ||
| AF_INET6 => unsafe { | ||
| let addr = (*(sock_addr as *mut SOCKADDR_IN6)).sin6_addr; | ||
| Some(IpAddr::from(addr.u.Byte().clone())) | ||
| }, | ||
| _ => { | ||
| // ignore unknown address families; only support IPv4/IPv6 | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
I think continueing here and advancing the pointer would again be cleaner so that you don't need to continually check for Somes. I'll have to review the rest of the code more closely on a Windows machine as well.
There was a problem hiding this comment.
Ok - I agree. I can fix this up. Let me know about the above and I'll change it all at once.
There was a problem hiding this comment.
I think that should solve my concerns for the time being then with the above feedback, I'll punt on the HashMap stuff for the time being (I want to do a bigger overhaul of the entire crate here at some point anyway so I'll investigate that closer then), so just let me know whenever things are updated and I'll merge 👍
Also added ./example/lookup_by_ip.rs which correctly outputs the MacAddress on my windows machine.
Linux implementation in the next commit.