Thanks for the review Ilya,
honestly I was skeptical about how could those prerequisite-patch-id
hashes work, but I saw it in other patch series, so I gave it a shot :D
Thanks for the formatting comments, I'll apply them in v4, but I'll
wait first for technical review.
Martin.
On Tue, 2025-01-21 at 15:03 +0100, Ilya Maximets wrote:
> 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
