RE: rte_fib network order bug

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Friday, 15 November 2024 14.02
> 
> Morten Brørup, Nov 14, 2024 at 15:35:
> >> RTE_IPV4 is only useful to define addresses in unit tests.
> >
> > There are plenty of special IP addresses and subnets, where a
> shortcut
> > macro makes the address easier readable in the code.
> 
> OK, let me reformulate. I didn't mean to say that RTE_IPV4 is useless.
> But it will always generate addresses in *host order*. Which means they
> cannot be used in IPv4 headers without passing them through htonl().
> This is weird in my opinion.

Robin, you've totally won me over on this endian discussion. :-)
Especially the IPv6 comparison make it clear why IPv4 should also be network byte order.

API/ABI stability is a pain... we're stuck with host endian IPv4 addresses; e.g. for the RTE_IPV4() macro, which I now agree produces the wrong endian value (on little endian CPUs).

> 
> >> Why would control plane use a different representation of addresses
> >> compared to data plane?
> >
> > Excellent question.
> > Old habit? Growing up using big endian CPUs, we have come to think of
> > IPv4 addresses as 32 bit numbers, so we keep treating them as such.
> > With this old way of thinking, the only reason to use network endian
> > in the fast path with little endian CPUs is for performance reasons
> > (to save the byte swap) - if not, we would still prefer using host
> > endian in the fast path too.
> 
> I understand the implementation reasons why you would prefer working
> with host order integers. But the APIs that deal with IPv4 addresses
> should not reflect implementation details.

They were probably designed based on the same way of thinking I was used to (until you convinced me I was wrong).

> 
> >> Also for consistency with IPv6, I really think
> >> that *all* addresses should be dealt in their network form.
> >
> > Food for thought!
> 
> Vladimir, could we at least consider adding a real network order mode
> for the rib and fib libraries? So that we can have consistent APIs
> between IPv4 and IPv6?

And/or rename RTE_FIB_F_NETWORK_ORDER to RTE_FIB_F_NETWORK_ORDER_LOOKUP or similar. This is important if real network order mode is added (now or later)!

> 
> On that same topic, I wonder if it would make sense to change the API
> parameters to use an opaque rte_ipv4_addr_t type instead of a native
> uint32_t to avoid any confusion.

It could be considered an IPv4 address type (like the IPv6 address type) (which should be in network endian), which it is not, so I don't like this idea.
What the API really should offer is a choice (or a union) of uint32_t and rte_be32_t, but that's not possible, so also using uint32_t for big endian values seems like a viable compromise.
Another alternative, using void* for the IPv4 address array, seems overkill to me, since compilers don't warn about mixing uint32_t with rte_be32_t values (like mixing signed and unsigned emits warnings).

> 
> Thanks!