|SUCCESS| [2/2] rib: add mode to include top-level route in traversal

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

89061829a23c (patch 1/2)
Author: Robin Jarry <rjarry at redhat.com>

rib: rename nxt flag parameter to mode

The flag parameter in rte_rib_get_nxt() and rte_rib6_get_nxt() is a
plain int used as a typed enumeration. Give the anonymous enums proper
names (rte_rib_nxt_mode, rte_rib6_nxt_mode) and rename the parameter
from "flag" to "mode" to reflect its purpose.

> diff --git a/lib/rib/rte_rib.c b/lib/rib/rte_rib.c
> index 046db131ca00..89061829a23c 100644
> --- a/lib/rib/rte_rib.c
> +++ b/lib/rib/rte_rib.c
> @@ -175,7 +175,7 @@ rte_rib_lookup_exact(struct rte_rib *rib, uint32_t ip, uint8_t depth)
>  RTE_EXPORT_SYMBOL(rte_rib_get_nxt)
>  struct rte_rib_node *
>  rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
> -	uint8_t depth, struct rte_rib_node *last, int flag)
> +	uint8_t depth, struct rte_rib_node *last, enum rte_rib_nxt_mode mode)

This API change looks correct.

> @@ -205,7 +205,7 @@ rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
>  				(is_covered(tmp->ip, ip, depth) &&
>  				(tmp->depth > depth))) {
>  			prev = tmp;
> -			if (flag == RTE_RIB_GET_NXT_COVER)
> +			if (mode == RTE_RIB_GET_NXT_COVER)

The implicit comparison of enum against zero is correct here. This is
proper style for enum values.

> diff --git a/lib/rib/rte_rib.h b/lib/rib/rte_rib.h
> index c325b3e3618c..0fabfb2a41a6 100644
> --- a/lib/rib/rte_rib.h
> +++ b/lib/rib/rte_rib.h
> @@ -26,11 +26,11 @@ extern "C" {
>  /**
>   * rte_rib_get_nxt() flags
>   */
> -enum {
> -	/** flag to get all subroutes in a RIB tree */
> +enum rte_rib_nxt_mode {
> +	/** get all subroutes in a RIB tree, excluding any exact match top-level route */
>  	RTE_RIB_GET_NXT_ALL,
> -	/** flag to get first matched subroutes in a RIB tree */
> -	RTE_RIB_GET_NXT_COVER
> +	/** get first matched subroutes in a RIB tree, excluding any exact match top-level route */
> +	RTE_RIB_GET_NXT_COVER,

Does adding a trailing comma to an existing enum in the API break binary
compatibility? The enum rte_rib_nxt_mode is now a public ABI type, but
there's no trailing comma in the original code. This might affect code
that relies on the original enum layout.

[ ... ]

a3c287e215ec (patch 2/2)
Author: Robin Jarry <rjarry at redhat.com>

rib: add mode to include top-level route in traversal

rte_rib_get_nxt() and rte_rib6_get_nxt() skip the exact match
top-level route when iterating subroutes. This is the expected behavior
in most cases but some users need the full subtree including the root.

Add a RTE_RIB_GET_NXT_ALL_TOP (and RTE_RIB6_GET_NXT_ALL_TOP) mode
that uses >= instead of > when comparing depths so that the top-level
route is returned as well.

> diff --git a/app/test/test_rib.c b/app/test/test_rib.c
> index a4a683140df3..f56490e67ec0 100644
> --- a/app/test/test_rib.c
> +++ b/app/test/test_rib.c

[ ... ]

> @@ -313,11 +314,26 @@ test_tree_traversal(void)
>  	node = rte_rib_insert(rib, ip2, depth);
>  	RTE_TEST_ASSERT(node != NULL, "Failed to insert rule\n");
>  
> +	node = rte_rib_insert(rib, 0, 0);
> +	RTE_TEST_ASSERT(node != NULL, "Failed to insert default rule\n");
> +
>  	node = NULL;
>  	node = rte_rib_get_nxt(rib, RTE_IPV4(10, 10, 130, 0), 24, node,
>  			RTE_RIB_GET_NXT_ALL);
>  	RTE_TEST_ASSERT(node != NULL, "Failed to get rib_node\n");
>  
> +	num = 0;
> +	node = NULL;
> +	while ((node = rte_rib_get_nxt(rib, 0, 0, node, RTE_RIB_GET_NXT_ALL)) != NULL)
> +		num++;
> +	RTE_TEST_ASSERT(num == 2, "Invalid number of routes\n");
> +
> +	num = 0;
> +	node = NULL;
> +	while ((node = rte_rib_get_nxt(rib, 0, 0, node, RTE_RIB_GET_NXT_ALL_TOP)) != NULL)
> +		num++;
> +	RTE_TEST_ASSERT(num == 3, "Default route not returned by rte_rib_get_nxt\n");

The new test code looks correct.

[ ... ]

> diff --git a/lib/rib/rte_rib.c b/lib/rib/rte_rib.c
> index 89061829a23c..a3c287e215ec 100644
> --- a/lib/rib/rte_rib.c
> +++ b/lib/rib/rte_rib.c
> @@ -167,6 +167,14 @@ rte_rib_lookup_exact(struct rte_rib *rib, uint32_t ip, uint8_t depth)
>  	return __rib_lookup_exact(rib, ip, depth);
>  }
>  
> +static bool
> +depth_match(struct rte_rib_node *node, uint8_t depth, int flag)

Does this code use int where enum rte_rib_nxt_mode should be used? The
function signature shows int flag but this should be enum rte_rib_nxt_mode
to match the API changes in the first patch.

> +{
> +	if (flag == RTE_RIB_GET_NXT_ALL_TOP)
> +		return node->depth >= depth;
> +	return node->depth > depth;
> +}
> +
>  /*
>   *  Traverses on subtree and retrieves more specific routes
>   *  for a given in args ip/depth prefix
> @@ -195,7 +203,7 @@ rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
>  			tmp = tmp->parent;
>  			if (is_valid_node(tmp) &&
>  					(is_covered(tmp->ip, ip, depth) &&
> -					(tmp->depth > depth)))
> +					(depth_match(tmp, depth, mode))))

Does the call chain rte_rib_get_nxt()->depth_match() properly handle
all enum values? The depth_match() function only explicitly checks
RTE_RIB_GET_NXT_ALL_TOP but receives mode as int. Can this cause issues
with RTE_RIB_GET_NXT_COVER?

> @@ -203,7 +211,7 @@ rte_rib_get_nxt(struct rte_rib *rib, uint32_t ip,
>  	while (tmp) {
>  		if (is_valid_node(tmp) &&
>  				(is_covered(tmp->ip, ip, depth) &&
> -				(tmp->depth > depth))) {
> +				(depth_match(tmp, depth, mode)))) {

Same question applies here in the second call site.

> diff --git a/lib/rib/rte_rib6.c b/lib/rib/rte_rib6.c
> index c23881f6247e..5114fb522e3e 100644
> --- a/lib/rib/rte_rib6.c
> +++ b/lib/rib/rte_rib6.c
> @@ -186,6 +186,14 @@ rte_rib6_lookup_exact(struct rte_rib6 *rib,
>  	return NULL;
>  }
>  
> +static bool
> +depth_match(struct rte_rib6_node *node, uint8_t depth, enum rte_rib6_nxt_mode mode)
> +{
> +	if (mode == RTE_RIB6_GET_NXT_ALL_TOP)
> +		return node->depth >= depth;
> +	return node->depth > depth;
> +}

This version correctly uses enum rte_rib6_nxt_mode but does this code
also need to explicitly handle RTE_RIB6_GET_NXT_COVER? The logic returns
true for any mode value that is not RTE_RIB6_GET_NXT_ALL_TOP which seems
to assume both RTE_RIB6_GET_NXT_ALL and RTE_RIB6_GET_NXT_COVER should
use the same depth comparison.

[ ... ]