Skip to content

Add ip neigh show#21

Draft
Gilnaa wants to merge 2 commits into
rust-netlink:mainfrom
Gilnaa:neigh-show
Draft

Add ip neigh show#21
Gilnaa wants to merge 2 commits into
rust-netlink:mainfrom
Gilnaa:neigh-show

Conversation

@Gilnaa
Copy link
Copy Markdown

@Gilnaa Gilnaa commented May 23, 2026

Draft PR, still missing some things, but wanted to get your feedback if possible.

  • Write tests
    • A test for device selection (dev, vrf, nomaster)
    • A test for unused
    • A test for proxy
    • A test for statistics (-s)
  • Statistics use wrong units. (Need to divide by USER_HZ, which is returned by sysconf(_SC_CLK_TCK) in C)
image image

Depends on:

@Gilnaa Gilnaa force-pushed the neigh-show branch 2 times, most recently from 5ec9ddb to 911c737 Compare May 23, 2026 15:04
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/ip/neighbour/show.rs
Comment on lines +419 to +422
neigh.probes = None;
}

neighbour_info.push(neigh);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

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.

Suggested change
neigh.probes = None;
}
neighbour_info.push(neigh);
if let Some(address_filter) = address_filter {
if neigh.dst != address_filter {
continue;
}
}

Comment thread src/ip/neighbour/cli.rs Outdated
Comment thread src/ip/neighbour/show.rs
let (address, radix) = if let Some(address) = address.strip_prefix("0x") {
(address, 16)
} else if let Some(address) = address.strip_prefix("0b") {
(address, 2)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

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 {

Comment thread src/ip/neighbour/show.rs
Comment thread src/ip/neighbour/show.rs Outdated
Comment thread src/ip/neighbour/show.rs Outdated
"failed" => NudFilter::Specified(NeighbourState::Failed),
"noarp" => NudFilter::Specified(NeighbourState::Noarp),
"permanent" => NudFilter::Specified(NeighbourState::Permanent),
"none" => NudFilter::Specified(NeighbourState::None),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's impl Display and From<&str> for NeightbourType in netlink-packet-route, so others can take it also.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Comment thread src/ip/neighbour/show.rs Outdated
}
}

while let Some(opt) = opts.next() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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;
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the meantime I'll extract the argument parsing and make it a bit prettier.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see, thank you for the clarification!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread src/ip/neighbour/show.rs Outdated
Ok(Some(cli_addr_info))
}

pub(crate) async fn handle_show(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function contains more 100+ lines of code which is normally a sign of bad design. Try to split it into small functions.

@cathay4t
Copy link
Copy Markdown
Member

USER_HZ can be retrieved by nix::unistd::sysconf(nix::unistd::SysconfVar::CLK_TCK)

Comment thread Cargo.toml Outdated
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.

2 participants