Hi Frode, Thanks for the review, I'll apply your suggestions. Please see couple of my follow-up comments below.
On Mon, 2025-01-20 at 09:37 +0100, Frode Nordahl wrote: > Hello, Martin, > > Thank you for catching this issue, and this change makes sense to me. > > The data structures are properly initialized and ovs_router_insert > copes just fine with getting an empty string for interface name and > in6addr_any for gw. > > On Fri, Jan 17, 2025 at 6:31 PM Martin Kalcok > <[email protected]> wrote: > > > > A recent commit[0] added condition to "route_table_parse__" > > function that > > A Fixes tag would probably be in order here and you can refer to it > in > the text. Patches with fixes tags show up in the overview of > patchwork, helping reviewers prioritize. ack > > > causes it to throw error when parsing route without "nexthop > > ...throw **an** error ack > > > information" (either RTA_OIF, RTA_GATEWAY, RTA_VIA or > > RTA_MULTIPATH). While > > this requirement is reasonable for regular routes, there are some > > route types > > that don't need nexthop. One of these types is "blackhole" route, > > that we intend > > s/don't/doesn't I may be wrong, but I think this "don't" is correct since the sentence talks in plural about multiple "route types" > ...types is **the** ack > > PS: The mentioned route type is likely problematic for inclusive > naming initiatives, while we cannot do anything with the kernel enum > naming, we can consider whether this description is required in the > commit message. I double-checked the documented linked on the OVS Inclusive Language page[0], and "black hole" is not on the list of words recommended to replace. > > > to use in OVN for route advertising (RTN_BLACKHOLE)[1]. > > > > This change does not enforce the above-mentioned condition for > > those special > > route types that don't require "nexthop information". > > > > [0] > > https://github.com/openvswitch/ovs/commit/91fc51106cfeff33901080ecb063f621eeb00f54 > > [1] > > https://mail.openvswitch.org/pipermail/ovs-dev/2025-January/419383.html > > > > Signed-off-by: Martin Kalcok <[email protected]> > > --- > > lib/route-table.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/lib/route-table.c b/lib/route-table.c > > index 8106dc93d..c232a388f 100644 > > --- a/lib/route-table.c > > +++ b/lib/route-table.c > > @@ -220,6 +220,24 @@ route_table_reset(void) > > } > > } > > > > + > > +/* Returns true if the given route requires nexhop information > > (output > > + interface, nexthop IP, ...). Returns false for special route > > types, like > > + blackhole, that don't need this information. > > Does this need specific mention, referring to the inclusive naming > discussion above. > > Every line of the multiline comment should start with an asterisk > ('*'). ack > > > + */ > > The end of the multi-line comment could likely fit at the end of the > line above. > > > +static bool > > +route_needs_nexthop(const struct rtmsg *rtm) { > > It is likely that this function will only ever look at rtm_type, why > not feed it rtm_type directly and perhaps rename the function to > 'route_type_needs_nexthop'? While I don't expect this function to get much usage outside of this patch, I thought that it'd still be bit more user friendly this way. You pass it `rtmsg` structure, the function looks at the correct member and tells you the answer. Whereas if it accepted `unsigned char` as the argument, It'd be less explicit about it's inputs and probably require more documentation to explain what exactly should be passed to the function. Do you see any downsides to accepting `rtmsg` structure as the argument? Thanks, Martin. [0] https://docs.openvswitch.org/en/latest/internals/contributing/inclusive-language/ > > -- > Frode Nordahl > > > + switch (rtm->rtm_type) { > > + case RTN_BLACKHOLE: > > + case RTN_THROW: > > + case RTN_UNREACHABLE: > > + case RTN_PROHIBIT: > > + return false; > > + default: > > + return true; > > + } > > +} > > + > > static int > > route_table_parse__(struct ofpbuf *buf, size_t ofs, > > const struct nlmsghdr *nlmsg, > > @@ -430,7 +448,7 @@ route_table_parse__(struct ofpbuf *buf, size_t > > ofs, > > &mp_change.rd.nexthops); > > } > > } > > - if (!attrs[RTA_OIF] && !attrs[RTA_GATEWAY] > > + if (route_needs_nexthop(rtm) && !attrs[RTA_OIF] && > > !attrs[RTA_GATEWAY] > > && !attrs[RTA_VIA] && !attrs[RTA_MULTIPATH]) { > > VLOG_DBG_RL(&rl, "route message needs an RTA_OIF, > > RTA_GATEWAY, " > > "RTA_VIA or RTA_MULTIPATH > > attribute"); > > -- > > 2.43.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
