On 1/21/25 11:44, Martin Kalcok wrote:
Hi, Martin. Thanks for the patch.
I'll let Eelco to reply with a technical review. But see some style and
formatting comments below.
Best regards, Ilya Maximets.
> A recent commit added condition to "route_table_parse__" function that
> causes it to throw an 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. We intend to use one of these types,
> (RTN_BLACKHOLE)[0], in OVN for route advertising .
>
> This change does not enforce the above-mentioned condition for those special
> route types that don't require "nexthop information".
>
> v3:
> * Fix typo in arguments for the "route_type_needs_nexthop" function
>
> v2:
> * Ensure that the list of nexthops is cleared if the route does not have
> them.
> * The function for determining whether a route requires nexthop now takes
> route_type (unsigned char) as an argument directly. Previously it
> took a pointer to rtmsg struct.
> * The patch is rebased on top of the in-flight patch series by Frode[1]
> * Fixed typos.
This should not be in the commit message. Patch version history should be
under the cut line ("---"), so it doesn't end up in git.
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2025-January/419383.html
> [1]
> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
>
> CC: Frode Nordahl <[email protected]>
> Fixes: 91fc51106cfe ("route-table: Support parsing multipath routes.")
> Signed-off-by: Martin Kalcok <[email protected]>
> ---
> lib/route-table.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/lib/route-table.c b/lib/route-table.c
> index 6dfb364a4..ecc7fd7e5 100644
> --- a/lib/route-table.c
> +++ b/lib/route-table.c
> @@ -226,6 +226,23 @@ route_table_reset(void)
> }
> }
>
> +/* Returns true if the given route requires nexthop information (output
> + * interface, nexthop IP, ...). Returns false for special route types
> + * that don't need this information. */
> +static bool
> +route_type_needs_nexthop(unsigned char rtmsg_type) {
'{' should be on a new line for function definitions.
> + switch (rtmsg_type) {
> + case RTN_BLACKHOLE:
> + case RTN_THROW:
> + case RTN_UNREACHABLE:
> + case RTN_PROHIBIT:
> + return false;
An empty line here.
> + default:
> + return true;
Cases should be on the same level with the switch.
> + }
> +}
> +
> +
One too many empty lines.
> static int
> route_table_parse__(struct ofpbuf *buf, size_t ofs,
> const struct nlmsghdr *nlmsg,
> @@ -450,13 +467,20 @@ route_table_parse__(struct ofpbuf *buf, size_t ofs,
> ofpbuf_uninit(&mp_buf);
> }
> }
> - if (!attrs[RTA_OIF] && !attrs[RTA_GATEWAY]
> - && !attrs[RTA_VIA] && !attrs[RTA_MULTIPATH]) {
> + if (route_type_needs_nexthop(rtm->rtm_type) && !attrs[RTA_OIF]
I'd suggest to break the line after the function call and have
two attr checks per line on subsequent lines.
> + && !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");
> goto error_out;
> }
> /* Add any additional RTA attribute processing before RTA_MULTIPATH.
> */
> +
> + /* Ensure that the change->rd->nexthops list is cleared in case that
> + * the route does not need a next hop. */
> + if (!route_type_needs_nexthop(rtm->rtm_type)) {
> + route_data_destroy_nexthops__(&change->rd);
> + }
> } else {
> VLOG_DBG_RL(&rl, "received unparseable rtnetlink route message");
> goto error_out;
>
> base-commit: caed64d1685409d1f9b53b35621a08ad6588200c
> prerequisite-patch-id: 9d606dcf57f9213291028029f2a71a43f346ce1e
> prerequisite-patch-id: b61606e7f32fede1bd34248957f67d72040be326
> prerequisite-patch-id: 907ead49b46ccf4b2e81e9f49f535788e592348b
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
