Re: rte_fib network order bug

["Robin Jarry" <rjarry@redhat.com>]

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?