On 21 Jan 2025, at 15:32, [email protected] wrote:
> 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.
I think it looks good (from visual inspection). Please go ahead with posting a
v4, and I’ll review it (and maybe Frode can have another look also).
Cheers,
Eelco
> 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
