|SUCCESS| fib,rib: implement full network order support for IPv4

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/164054

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-18

This patch introduces network byte order support for IPv4 addresses in the FIB and RIB libraries. I'll review it for correctness bugs, coding style issues, and API requirements.

git sha: (not provided in patch)
Author: Robin Jarry <rjarry at redhat.com>
Subject: [PATCH dpdk] fib,rib: implement full network order support for IPv4

The patch extends network byte order handling in FIB/RIB from lookup-only
to all user-facing API functions. It renames RTE_FIB_F_LOOKUP_NETWORK_ORDER
to RTE_FIB_F_NETWORK_ORDER and adds RTE_RIB_F_NETWORK_ORDER.

> diff --git a/lib/fib/dir24_8.c b/lib/fib/dir24_8.c
> index 489d2ef427c6..781ecbe9e64e 100644
> --- a/lib/fib/dir24_8.c
> +++ b/lib/fib/dir24_8.c
> @@ -414,24 +414,33 @@ install_to_fib(struct dir24_8_tbl *dp, uint32_t ledge, uint32_t redge,
>  	return 0;
>  }
>  
> +/*
> + * modify_fib operates in host byte order. When the RIB carries the
> + * network-order flag, rte_rib_get_nxt/rte_rib_get_ip return addresses
> + * in network order, so this helper converts them back to host order
> + * for DIR24_8 table operations.
> + */
>  static int
>  modify_fib(struct dir24_8_tbl *dp, struct rte_rib *rib, uint32_t ip,
>  	uint8_t depth, uint64_t next_hop)
>  {
>  	struct rte_rib_node *tmp = NULL;
>  	uint32_t ledge, redge, tmp_ip;
> +	uint32_t rib_ip = dp->be_addr ? rte_cpu_to_be_32(ip) : ip;
>  	int ret;
>  	uint8_t tmp_depth;
>  
>  	ledge = ip;
>  	do {
> -		tmp = rte_rib_get_nxt(rib, ip, depth, tmp,
> +		tmp = rte_rib_get_nxt(rib, rib_ip, depth, tmp,

Does this code handle the depth parameter correctly with network order?
The depth is a prefix length that should be byte-order independent, but
does rte_rib_get_nxt() expect it to work correctly when the IP is in
network order?

[ ... ]

> @@ -606,6 +622,7 @@ dir24_8_create(const char *name, int socket_id, struct rte_fib_conf *fib_conf)
>  	dp->def_nh = def_nh;
>  	dp->nh_sz = nh_sz;
>  	dp->number_tbl8s = num_tbl8;
> +	dp->be_addr = !!(fib_conf->flags & RTE_FIB_F_NETWORK_ORDER);

Is dp->be_addr being validated against the RIB's network order flag?
When creating a FIB with RTE_FIB_F_NETWORK_ORDER, does the code ensure
that its internal RIB object has RTE_RIB_F_NETWORK_ORDER set? This
consistency is mentioned in the commit message but should be verified.

> diff --git a/lib/fib/rte_fib.c b/lib/fib/rte_fib.c
> index 184210f38070..3e4c15cfc809 100644
> --- a/lib/fib/rte_fib.c
> +++ b/lib/fib/rte_fib.c
> @@ -172,6 +172,8 @@ rte_fib_create(const char *name, int socket_id, struct rte_fib_conf *conf)
>  
>  	rib_conf.ext_sz = conf->rib_ext_sz;
>  	rib_conf.max_nodes = conf->max_routes * 2;
> +	rib_conf.flags = (conf->flags & RTE_FIB_F_NETWORK_ORDER) ?
> +		RTE_RIB_F_NETWORK_ORDER : 0;

Does this code properly preserve any other RIB flags that might already
be set in rib_conf.flags? The assignment here overwrites rib_conf.flags
completely rather than using |= to preserve existing flags.

> diff --git a/lib/fib/rte_fib.h b/lib/fib/rte_fib.h
> index b16a653535cf..5542e735b6f3 100644
> --- a/lib/fib/rte_fib.h
> +++ b/lib/fib/rte_fib.h
> @@ -91,9 +91,11 @@ enum rte_fib_lookup_type {
>  	/**< Vector implementation using AVX512 */
>  };
>  
> -/** If set, fib lookup is expecting IPv4 address in network byte order */
> -#define RTE_FIB_F_LOOKUP_NETWORK_ORDER 1
> -#define RTE_FIB_ALLOWED_FLAGS (RTE_FIB_F_LOOKUP_NETWORK_ORDER)
> +/** If set, all user-facing IPv4 addresses are in network byte order */
> +#define RTE_FIB_F_NETWORK_ORDER 1
> +#define RTE_FIB_F_LOOKUP_NETWORK_ORDER \
> +	(RTE_DEPRECATED(RTE_FIB_F_LOOKUP_NETWORK_ORDER) RTE_FIB_F_NETWORK_ORDER)
> +#define RTE_FIB_ALLOWED_FLAGS (RTE_FIB_F_NETWORK_ORDER)

Is the deprecation notice syntactically correct? The RTE_DEPRECATED
macro typically takes a string message as argument, not just a name.
Should this be RTE_DEPRECATED("use RTE_FIB_F_NETWORK_ORDER instead")?

> diff --git a/lib/rib/rte_rib.c b/lib/rib/rte_rib.c
> index 046db131ca00..c6dbe684d211 100644
> --- a/lib/rib/rte_rib.c
> +++ b/lib/rib/rte_rib.c
> @@ -28,6 +29,8 @@ static struct rte_tailq_elem rte_rib_tailq = {
>  EAL_REGISTER_TAILQ(rte_rib_tailq)
>  
>  #define RTE_RIB_VALID_NODE	1
> +#define RIB_NODE_NET_ORDER	2
> +#define RTE_RIB_ALLOWED_FLAGS	(RTE_RIB_F_NETWORK_ORDER)

Is there a potential collision between RIB_NODE_NET_ORDER and other
node flag bits? The code uses node->flag to store both RTE_RIB_VALID_NODE
and RIB_NODE_NET_ORDER, but are these flag bits properly isolated?

> @@ -257,12 +285,18 @@ rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
>  	int d = 0;
>  	uint32_t common_prefix;
>  	uint8_t common_depth;
> +	uint8_t net_order = 0;
>  
>  	if (unlikely(rib == NULL || depth > RIB_MAXDEPTH)) {
>  		rte_errno = EINVAL;
>  		return NULL;
>  	}
>  
> +	if (rib->flags & RTE_RIB_F_NETWORK_ORDER) {
> +		ip = rte_be_to_cpu_32(ip);
> +		net_order = RIB_NODE_NET_ORDER;
> +	}
> +

Does this conversion occur at the right time? The ip is masked with
rte_rib_depth_to_mask(depth) later in the function. Should the conversion
happen before or after masking?

> @@ -300,7 +334,7 @@ rte_rib_insert(struct rte_rib *rib, uint32_t ip, uint8_t depth)
>  		 */
>  		if ((ip == (*tmp)->ip) && (depth == (*tmp)->depth)) {
>  			node_free(rib, new_node);
> -			(*tmp)->flag |= RTE_RIB_VALID_NODE;
> +			(*tmp)->flag |= RTE_RIB_VALID_NODE | net_order;

Does this code handle the case where the existing node has different
byte order flags than the new insertion? Can we OR in net_order to a
node that might have been created with a different byte order setting?

> @@ -362,6 +396,8 @@ rte_rib_get_ip(const struct rte_rib_node *node, uint32_t *ip)
>  		return -1;
>  	}
>  	*ip = node->ip;
> +	if (node->flag & RIB_NODE_NET_ORDER)
> +		*ip = rte_cpu_to_be_32(*ip);

Is this the correct approach for rte_rib_get_ip()? The function takes
only a node pointer, not a rib pointer, so it relies on per-node flags.
Can this lead to inconsistencies if nodes from different RIBs with
different byte order settings are mixed?

> @@ -541,9 +579,9 @@ rte_rib_free(struct rte_rib *rib)
>  
>  	rte_mcfg_tailq_write_unlock();
>  
> -	while ((tmp = rte_rib_get_nxt(rib, 0, 0, tmp,
> +	while ((tmp = __rib_get_nxt(rib, 0, 0, tmp,
>  			RTE_RIB_GET_NXT_ALL)) != NULL)
> -		rte_rib_remove(rib, tmp->ip, tmp->depth);
> +		__rib_remove(rib, tmp->ip, tmp->depth);

Does this code properly handle the byte order conversion when freeing
the RIB? The __rib_remove() function now expects host byte order, and
tmp->ip is stored in host order, so this should be correct, but can
you verify this call chain?