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. > causes it to throw error when parsing route without "nexthop ...throw **an** error > 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 ...types is **the** 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. > 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 ('*'). > + */ 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'? -- 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
