[1/3] [nat] move PortRange to struct - #334
Conversation
76fd9df to
a01c3aa
Compare
internet-diglett
left a comment
internet-diglett
left a comment
There was a problem hiding this comment.
Overall looks good! Just a few comments.
| pub high: u16, | ||
| #[derive(PartialEq)] | ||
| pub(crate) struct Ipv6NatEntry { | ||
| pub ports: PortRange, |
There was a problem hiding this comment.
minor nit: port is a very overloaded term inside of dendrite (grepping for ports gives a lot of unrelated results). Maybe we should be more explicit and name this field port_range? I also notice that there is a convention of naming fields that are a single item thing and naming collections / iterators of a type thing**s**, so this also avoids creating confusion there.
There was a problem hiding this comment.
I think we use "L4 port" in Omicron and OPTE similar situations, which might be a good option.
There was a problem hiding this comment.
Are you suggesting PortRange be called L4PortRange?
There was a problem hiding this comment.
Yeah, sorry if that wasn't clear. One could also rename the ports field to something like l4_ports.
There was a problem hiding this comment.
Just making sure. Sounds good to me 👍
|
|
||
| #[derive(Clone, PartialEq)] | ||
| pub(crate) struct Ipv4NatEntry { | ||
| pub ports: PortRange, |
| assert_eq!(first_mapping(2, 2), Some(0)); | ||
| assert_eq!(first_mapping(4, 5), Some(0)); | ||
| assert_eq!(first_mapping(5, 6), None); | ||
| assert_eq!(first_mapping(5, 7), Some(1)); | ||
| assert_eq!(first_mapping(2, 6), Some(0)); | ||
| assert_eq!(first_mapping(5, 5), None); | ||
| assert_eq!(first_mapping(5, 20), Some(1)); | ||
| assert_eq!(first_mapping(12, 12), Some(2)); | ||
| assert_eq!(first_mapping(18, 18), Some(2)); | ||
| assert_eq!(first_mapping(19, 19), None); | ||
| assert_eq!(first_mapping(19, 40), None); | ||
| assert_eq!(first_mapping(0, 0), None); | ||
| assert_eq!(first_mapping(0, 2), Some(0)); | ||
| assert_eq!(space(0, 0), Some(0)); | ||
| assert_eq!(space(0, 1), None); | ||
| assert_eq!(space(11, 11), Some(2)); | ||
| assert_eq!(space(19, 32), Some(3)); | ||
| assert_eq!(space(0, 2), None); | ||
| assert_eq!(space(3, 5), None); | ||
| assert_eq!(space(3, 8), None); |
There was a problem hiding this comment.
Could this be a good candidate for property based testing?
There was a problem hiding this comment.
I guess so, but in this case it would be enforcing a property that's basically the same as what we already have in the code. I like to think of tests like this as a way of giving the developer a feel for how it works more so than actually exercising functionality.
There was a problem hiding this comment.
I guess that was a roundabout way of me asking "it seems we need a lot of cases to exercise this code... are we sure we've covered all of them?"
(I'm actually not sure either way, it just stood out to me)
There was a problem hiding this comment.
I just adapted the tests that were already there since this purposefully a quite mechanical PR with no new semantics.
3 participants
This PR:
PortRangetrait into a struct, validating thelow <= highinvariant by construction.overlapfunction.This is the first of 3 PRs simplifying and de-duplicating some of the
nat.rscode.PortRangeto struct #334NatAddresstrait #335Left for future work:
PortRangeusage further.