Hi Eelco, thanks for the feedback, I have a follow-up question, just to make sure that I understand your comment correctly.
On Mon, 2025-01-20 at 09:59 +0100, Eelco Chaudron wrote: > > > On 17 Jan 2025, at 18:30, Martin Kalcok wrote: > > > A recent commit[0] added condition to "route_table_parse__" > > function that > > causes it to throw error when parsing route without "nexthop > > 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 > > 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 > > Thanks for the patch Martin. I think you should include the fixes tag > for commit [0]. ack > > In addition, the logic of the route_table_parse__() function should > also change, as it should have an uninitialized/unused next hop in > the list. IIUC, the route_table_parse__ already unconditionally initializes empty list for "nexthops"[0]. Do you suggest that in the case that the route doesn't need a "real nexthop", we should add a dummy nexthop element to the list? Is it so that the route satisfies this[1] condition in route_table_handle_msg? Thanks, Martin. [0] https://github.com/openvswitch/ovs/blob/caed64d1685409d1f9b53b35621a08ad6588200c/lib/route-table.c#L276 [1] https://github.com/openvswitch/ovs/blob/caed64d1685409d1f9b53b35621a08ad6588200c/lib/route-table.c#L502-L503 > > Cheers, > > Eelco > > > 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. > > + */ > > +static bool > > +route_needs_nexthop(const struct rtmsg *rtm) { > > + 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
