Add ip networks for interface #1218
Conversation
|
|
|
Yes, it's "normal". Don't worry about it. ^^' |
|
Thanks for working on this! How complicated would it be to not use crates for getting this feature? And also, please add a test. |
|
I will try for not use crates and add tests |
|
If it's too tricky, it can be done as a second step. The most important part here is the test. |
|
It will be wonderful if not use crates for getting this feature. Worth to try. |
|
Definitely agree. 👍 |
|
@GuillaumeGomez crates removed. I added some tests for thie feature. Not sure tests is enough or not. |
| ) | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
Please add a test to ensure that supported platforms have at least one IP network for an interface. You can filter out using:
if !crate::IS_SUPPORTED_SYSTEM {
return;
}There was a problem hiding this comment.
Sure. Check added.
|
|
||
| // Safety: addrs.as_mut_ptr() is valid, it points to addrs. | ||
| if unsafe { libc::getifaddrs(addrs.as_mut_ptr()) } != 0 { | ||
| return ifaces; |
There was a problem hiding this comment.
Please add a sysinfo_debug call here to display the returned error saying that getifaddrs failed.
There was a problem hiding this comment.
Hope this call will help for debug when need
src/unix/network_helper.rs
Outdated
| @@ -116,3 +126,261 @@ pub(crate) fn get_interface_address() -> Result<InterfaceAddressIterator, String | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn get_interface_ip_networks() -> HashMap<String, HashSet<IpNetwork>> { | |||
There was a problem hiding this comment.
I think it'd be better to mark the entire function as unsafe: unsafe isn't only limited to a C call or a pointer access but "lives" until the unsafe data has been converted into Rust so to speak. So having local unsafe blocks doesn't make much sense.
There was a problem hiding this comment.
Now use inner function for unsafe code.
There was a problem hiding this comment.
Please make the function itself unsafe, don't wrap, it's giving the impression that the function is safe although it's not. ^^'
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
These tests are very appreciated, well done! 👍
|
A few small things to update and a test to add and then it'll be ready for merge. :) |
|
Update and tests all pushed.Thanks for your advice, much learned. |
src/common.rs
Outdated
|
|
||
| /// return the addr of the IpNetwork | ||
| pub fn addr(&self) -> IpAddr { | ||
| self.addr |
There was a problem hiding this comment.
At this point, better make the fields public (and document what they are).
src/common.rs
Outdated
|
|
||
| impl IpNetwork { | ||
| /// build new IpNetwork | ||
| pub fn new(addr: IpAddr, prefix: u8) -> Self { |
There was a problem hiding this comment.
With the fields public, this constructor won't be needed anymore too.
There was a problem hiding this comment.
Now I removed this constructor.
src/lib.rs
Outdated
| return; | ||
| } | ||
| let networks = Networks::new_with_refreshed_list(); | ||
| if networks.is_empty() || networks.iter().any(|(_, n)| !n.ip_networks().is_empty()) { |
There was a problem hiding this comment.
Why checking networks.is_empty()? Isn't it an issue if no networks are present?
There was a problem hiding this comment.
I once encountered a situation where there was no network interface in vmware windows virtual machine.Not checking networks.is_empty() is better for find problems.
There was a problem hiding this comment.
Do you mind removing this test to check if it fails in the CI please?
|
All updates pushed. |
|
Great work! Thanks a lot for this new feature! |
|
I've been waiting for this feature, thanks for implementing it! Is there any possibility of a release on crates.io in the near future? |
|
Since it's an API addition, I'll make a new big release. I'm taking this opportunity to make other breaking changes. |
Add IP networks for interface for #1104