Refactor InterfaceIndex type #879
Conversation
daniel-noland
force-pushed
the
pr/daniel-noland/interface-index
branch
3 times, most recently
from
cc0715a to
ff003df
Compare
daniel-noland
requested review from
sergeymatov
and removed request for
a team
| Ok(ifindex) => ifindex, | ||
| Err(err) => { | ||
| let msg = format!("{err}"); | ||
| error!("{err}"); |
There was a problem hiding this comment.
Do we want some more context here about where this error is coming from rather than just emiiting the err message?
routing/src/atable/resolver.rs
Outdated
| if interface.name == name { | ||
| InterfaceIndex::try_new(interface.index) | ||
| .map_err(|e| error!("{e}")) | ||
| .ok() |
There was a problem hiding this comment.
Does this now need to really return a Result<Option<InterfaceIndex>>? It should really never be the case that something is in the list of interfaces but its index is invalid right? And so shouldn't we single out that case? If that is too cumbersome, I suppose the error! will have to suffice. However, do we want some context in the text as to where this error happened instead of blindly logging e?
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the InterfaceIndex type from a basic u32 alias (IfIndex) to a proper type wrapper (net::interface::InterfaceIndex). This change improves type safety by ensuring interface indices cannot be zero and provides better validation through the try_new() constructor.
- Replaces
IfIndexalias with properInterfaceIndextype throughout the codebase - Adds validation to prevent zero interface indices via
InterfaceIndex::try_new() - Updates all interface-related APIs to use the new type
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| routing/src/rpc_adapt.rs | Updates RouteNhop conversion to validate interface indices and remove zero-index handling |
| routing/src/rib/vrftable.rs | Updates tests and VRF operations to use InterfaceIndex type |
| routing/src/rib/vrf.rs | Updates next-hop building functions to use InterfaceIndex |
| routing/src/rib/rib2fib.rs | Updates packet instruction building to use InterfaceIndex |
| routing/src/rib/nexthop.rs | Changes NhopKey to use InterfaceIndex instead of u32 |
| routing/src/interfaces/ | Updates interface table and management APIs to use InterfaceIndex |
| routing/src/fib/fibobjects.rs | Updates egress objects to use InterfaceIndex |
| routing/src/errors.rs | Updates error types to use InterfaceIndex |
| routing/src/cpi.rs | Adds validation for interface indices in RPC operations |
| routing/src/config/ | Updates configuration handling to use InterfaceIndex |
| routing/src/atable/ | Updates adjacency table to use InterfaceIndex |
| net/src/packet/ | Updates packet metadata to use InterfaceIndex |
| mgmt/src/processor/confbuild/router.rs | Adds validation when building router interface configs |
| dataplane/src/packet_processor/ | Updates packet processing to use InterfaceIndex |
| dataplane/src/drivers/kernel.rs | Updates kernel driver to use InterfaceIndex |
| config/src/errors.rs | Adds Invalid configuration error variant |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
routing/src/rib/rib2fib.rs
Outdated
| instructions.push(PktInstruction::Local( | ||
| self.key | ||
| .ifindex | ||
| .unwrap_or(InterfaceIndex::try_new(1).unwrap_or_else(|_| unreachable!())), |
There was a problem hiding this comment.
The nested unwrap_or_else with unreachable! is confusing. If InterfaceIndex::try_new(1) can never fail, use unwrap() directly. If it can fail, handle the error properly.
| .unwrap_or(InterfaceIndex::try_new(1).unwrap_or_else(|_| unreachable!())), | |
| .unwrap_or(InterfaceIndex::try_new(1).unwrap()), |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
the intent here is to signal that the error branch is genuinely unreachable.
I think this is ok (if verbose)
There was a problem hiding this comment.
that said, I do have a better option here: drop if there is no target interface for the packet. It makes no sense to locally deliver a packet when you don't know where it should be delivered to.
daniel-noland
force-pushed
the
pr/daniel-noland/interface-index
branch
2 times, most recently
from
327fe7d to
a4b5313
Compare
daniel-noland
force-pushed
the
pr/daniel-noland/interface-index
branch
from
a4b5313 to
11b6a0b
Compare
Early on in the project we ended up with multiple interface index types. This can be extremely confusing in the context of DPDK. Worse, the longer we wait, the more instances of each of these types we end up with. Time to fix this. Signed-off-by: Daniel Noland <[email protected]>
Signed-off-by: Daniel Noland <[email protected]>
Hopefully this makes trace logs a little easier to reason about. Signed-off-by: Daniel Noland <[email protected]>
We just don't need to clone here. Signed-off-by: Daniel Noland <[email protected]>
daniel-noland
force-pushed
the
pr/daniel-noland/interface-index
branch
from
11b6a0b to
f6d1797
Compare
2 participants
Long overdue tech debt payment