RE: rte_fib network order bug

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi Robin,

I like the second approach with one more suggestion. It would be nice to have 2 different flags - an existing flag (RTE_FIB_F_LOOKUP_NETWORK_ORDER) for the data plane bswap, and a new one for the control plane operations (smth like RTE_FIB/RIB_F_CP_NETWORK_ORDER).
Also for user convenience RTE_FIB_F_NETWORK_ORDER may be introduced as (RTE_FIB_F_LOOKUP_NETWORK_ORDER| RTE_FIB_F_CP_NETWORK_ORDER)

> There would need to be an additional RTE_IPV4_BE() macro to declare IPv4 addresses in network order.
This may be useful as well

Thanks!

-----Original Message-----
From: Robin Jarry <rjarry@redhat.com> 
Sent: Friday, November 22, 2024 4:15 PM
To: Vladimir Medvedkin <medvedkinv@gmail.com>; Stephen Hemminger <stephen@networkplumber.org>
Cc: Morten Brørup <mb@smartsharesystems.com>; Medvedkin, Vladimir <vladimir.medvedkin@intel.com>; dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Marchand, David <david.marchand@redhat.com>; Thomas Monjalon <thomas@monjalon.net>; Konstantin Ananyev <konstantin.ananyev@huawei.com>
Subject: Re: rte_fib network order bug

Vladimir Medvedkin, Nov 17, 2024 at 16:04:
> [Robin]
>> I had not understood that it was *only* the lookups that were network 
>> order
>
> [Morten]
>> When I saw the byte order flag the first time, it was not clear to me 
>> either that it only affected lookups - I too thought it covered the 
>> entire API of the library. This needs to be emphasized in the 
>> description of the flag. And the flag's name should contain LOOKUP 
>> [Morten] > And/or rename RTE_FIB_F_NETWORK_ORDER to 
>> RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar.
>
> There is a clear comment for this flag that it has effects on lookup. 
> Repeating the statement with an exclamation mark seems too much. 
> Moreover, at first this flag was named "RTE_FIB_FLAG_LOOKUP_BE" and it 
> was suggested for renaming here:
> https://inbox.dpdk.org/dev/D4SWPKOPRD5Z.87YIET3Y4AW@redhat.com/

This is my bad then. I had misunderstood what this flag was for. 
I should have been more careful. You had clearly stated that it was only affecting the lookup.

> So, feel free to submit patches adding this feature to the control 
> plane API, but let's consider:

I can commit to working on that topic if we can get a consensus. In my opinion there are two different approaches:

1) Change all IPv4 routing *APIs* to only use network order addresses =====================================================================

This would make them consistent with all networking stacks (linux, vpp, bsd, etc.) and would avoid confusion from users (like me) who naively used these libraries with addresses generated with inet_pton() or addresses taken verbatim from IPv4 packet headers.

More importantly, it would make them consistent on big-endian and little-endian architectures. Currently, the same code could work (without any byte swap) on aarch4, but would not work on x86_64.

It would also make them consistent with their IPv6 counterparts which do not require any byteswap.

This would be a drastic and breaking change but I think this would be the better solution in the long run.

To ensure that potential users of these libraries will not miss this change, the uint32_t parameters should be changed to a rte_ipv4_addr structure that follows the same idea than rte_ipv6_addr.

We could also simply use rte_be32_t types everywhere but it would expose potential users of these APIs with bugs that could not be found at compilation.

Internally, all these routing libraries would continue using host order integers, the changes I am suggesting only affect the public API.

2) Implement network order via opt-in flags ===========================================

This would allow the same thing as solution 1) but would keep the default behaviour which I find confusing and inconsistent with IPv6 and with all IPv4 networking stacks that I know.

The other concern I have with that second solution is that the public APIs would continue using uint32_t parameters which would be only correct when the network-order mode is not enabled.

On the other hand, it does not break any API for users that do not use the flags.

There would need to be an additional RTE_IPV4_BE() macro to declare IPv4 addresses in network order.

Any thoughts?