On 20 Jan 2025, at 13:07, [email protected] wrote:
> 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? The function adds an entry to the list unconditionally, which should not happen in this case where there are no next-hops. See ‘ovs_list_insert(&change->rd.nexthops, &rdnh->nexthop_node);’ > 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
