Add ip neigh show#21
Conversation
5ec9ddb to
911c737
Compare
There was a problem hiding this comment.
Code Review
This pull request implements the ip neighbour command, allowing users to view and filter neighbour/ARP table entries. The implementation includes support for various filters like device, VRF, and neighbour state, as well as a global statistics flag. Feedback focuses on ensuring compatibility with stable Rust by avoiding unstable features like let_chains, correcting the command's description, and improving robustness through better address parsing and logging instead of using println! or returning hard errors for malformed netlink messages.
| neigh.probes = None; | ||
| } | ||
|
|
||
| neighbour_info.push(neigh); |
There was a problem hiding this comment.
The use of let_chains (e.g., if let Some(...) && condition) is an unstable feature in Rust. Unless this project explicitly requires a nightly compiler and has enabled the feature, this will cause a compilation error on stable Rust. It is safer to use a nested if or match.
| neigh.probes = None; | |
| } | |
| neighbour_info.push(neigh); | |
| if let Some(address_filter) = address_filter { | |
| if neigh.dst != address_filter { | |
| continue; | |
| } | |
| } |
| let (address, radix) = if let Some(address) = address.strip_prefix("0x") { | ||
| (address, 16) | ||
| } else if let Some(address) = address.strip_prefix("0b") { | ||
| (address, 2) |
There was a problem hiding this comment.
The current address parsing logic does not handle octal representation (leading zero). While hex (0x) and binary (0b) are supported, a string like 0123 will be parsed as decimal 123 instead of octal 83. It is recommended to check for a leading zero and use radix 8.
} else if address.starts_with('0') && address.len() > 1 {
(address, 8)
} else {| } | ||
| } | ||
|
|
||
| while let Some(opt) = opts.next() { |
There was a problem hiding this comment.
We should use clap options here instead accepting arbitrary option here.
The iproute2 code indicate these filters can be done by set proper value in request netlink message, so kernel can do the filter for us by not returning unmatched entries.
It might looks like:
if matches.contains_id("proxy") {
req.ndm.ndm_flags |= NTF_PROXY;
}
There was a problem hiding this comment.
Am I allowed to read their code?
I assumed there's a licensing issue and so only prodded iproute2 with strace,
but maybe I'm just making it too hard on myself.
I already set NTF_PROXY by calling NeighbourGetRequest::proxies
This is the only flag the kernel allows, and it also does not support NUD-filtering.
See https://github.com/torvalds/linux/blob/4cbfe4502e3d4bda48eb4b83dfad8d7da3b22e90/net/core/neighbour.c#L2903
To summerise, it allows for NTF_PROXY, NDA_IFINDEX, and NDA_MASTER. (and this PR uses all 3).
When you do ask for a specific nud, iproute2 read everything and discards:
levi-win11# ip neigh
20.0.0.1 dev foo lladdr aa:aa:aa:aa:aa:aa PERMANENT
levi-win11# ip neigh show nud reachable
levi-win11#
levi-win11# strace -e sendto,recvmsg ip neigh show nud reachable
sendto(3, [[{nlmsg_len=28, nlmsg_type=RTM_GETNEIGH, nlmsg_flags=NLM_F_REQUEST|NLM_F_DUMP, nlmsg_seq=1779601713, nlmsg_pid=0}, {ndm_family=AF_UNSPEC, ndm_ifindex=0, ndm_state=NUD_NONE, ndm_flags=0, ndm_type=RTN_UNSPEC}], {nlmsg_len=0, nlmsg_type=0 /* NLMSG_??? */, nlmsg_flags=0, nlmsg_seq=0, nlmsg_pid=0}], 284, 0, NULL, 0) = 284
recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base=NULL, iov_len=0}], msg_iovlen=1, msg_controllen=0, msg_flags=MSG_TRUNC}, MSG_PEEK|MSG_TRUNC) = 96
recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base=[[{nlmsg_len=76, nlmsg_type=RTM_NEWNEIGH, nlmsg_flags=NLM_F_MULTI, nlmsg_seq=1779601713, nlmsg_pid=2172648}, {ndm_family=AF_INET, ndm_ifindex=if_nametoindex("foo"), ndm_state=NUD_PERMANENT, ndm_flags=0, ndm_type=RTN_BROADCAST}, [[{nla_len=8, nla_type=NDA_DST}, inet_addr("20.0.0.1")], [{nla_len=10, nla_type=NDA_LLADDR}, aa:aa:aa:aa:aa:aa], [{nla_len=8, nla_type=NDA_PROBES}, 0], [{nla_len=20, nla_type=NDA_CACHEINFO}, {ndm_confirmed=5544120, ndm_used=5544120, ndm_updated=5544120, ndm_refcnt=0}]]], [{nlmsg_len=20, nlmsg_type=NLMSG_DONE, nlmsg_flags=NLM_F_MULTI, nlmsg_seq=1779601713, nlmsg_pid=2172648}, 0]], iov_len=32768}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 96
I agree that it would make sense for the kernel to filter this; I assume it never came up as a performance edge case.
I would love to use clap, but my understanding is that it does not allow for unhyphenated-values the same way iproute2 does.
There was a problem hiding this comment.
In the meantime I'll extract the argument parsing and make it a bit prettier.
There was a problem hiding this comment.
My bad. clap does not support this kind of arg parsing.
For license issue, I am not lawyer. But knowledge is not licensed, we read iproute2 to understand how kernel handle these filters, then use that knowledge to produce Rust code which is completely different from iproute2 C code.
There was a problem hiding this comment.
I see, thank you for the clarification!
There was a problem hiding this comment.
I had similar arg parsing code at https://github.com/rust-netlink/iproute-rs/blob/main/src/ip/link/add.rs#L69 . Let me know if you have better idea.
|
USER_HZ can be retrieved by |
69c04ef to
0f47f8e
Compare
|
@cathay4t The only test missing is for A bit rough to test because we cannot compare strings directly, as calling iproute2 and then iproute-rs will produce different values due to timing. Will work on it later by stripping the numbers, comparing the rest of the string, and making sure the number are within a reasonable difference. |
|
There is |
|
Done. |
|
No apologies at all. You are doing great by donating your time and effort on open source projects. |
|
In case you want to work on more adoptions, my local branches has full support of |
|
I'll probably continue with Although currently I'm bothered with how manual the argument parsing was, might tackle that and something a bit less cumbersome. |
|
Yeah, iproute2 has special argument parsing. you may consider proc-macro2 because linux kernel is using it https://rust.docs.kernel.org/proc_macro2/index.html . I recall proc-macro is marked as Considering the argument parsing is one-time work(once done, no changes required), like netlink protocol, instead of using fancy hard to maintain, deduplicated code, I incline to easy to read, duplicated code. But I will not against you using proc-macros2. |
Draft PR, still missing some things, but wanted to get your feedback if possible.
dev,vrf,nomaster)unusedproxy-s)sysconf(_SC_CLK_TCK)in C)Depends on: