|SUCCESS| [v2, 23/23] examples/l3fwd-power: resolve shadow variable warnings

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-08

I'll review this DPDK patch series focusing on the guidelines and code quality issues.

git sha: 20260407151732.272195-2-stephen at networkplumber.org (and subsequent patches in series)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH v2 01/23] examples/ethtool: resolve shadow variable warnings

This is a patch series that addresses shadow variable warnings across multiple DPDK example applications by renaming variables and parameters that shadow global or outer-scope declarations.

> diff --git a/examples/ethtool/ethtool-app/main.c b/examples/ethtool/ethtool-app/main.c
> index b6bbae70d2..6545eb322d 100644
> --- a/examples/ethtool/ethtool-app/main.c
> +++ b/examples/ethtool/ethtool-app/main.c
> @@ -86,7 +86,7 @@ void mark_port_newmac(int idx_port)
>  	ptr_port->port_dirty = 1;
>  }
>  
> -static void setup_ports(struct app_config *app_cfg, int cnt_ports)
> +static void setup_ports(struct app_config *ptr_cfg, int cnt_ports)

Does this parameter name choice follow a consistent naming convention? The 
code uses both app_cfg (global) and ptr_cfg (parameter). Would cfg_param or 
config_ptr be clearer to distinguish parameter from global?

> @@ -101,7 +101,7 @@ static void setup_ports(struct app_config *app_cfg, int cnt_ports)
>  	cfg_port.txmode.mq_mode = RTE_ETH_MQ_TX_NONE;
>  
>  	for (idx_port = 0; idx_port < cnt_ports; idx_port++) {
> -		struct app_port *ptr_port = &app_cfg->ports[idx_port];
> +		struct app_port *ptr_port = &ptr_cfg->ports[idx_port];

[ ... ]

> diff --git a/examples/bond/main.c b/examples/bond/main.c
> index 4e8eeb7a5e..99359cd6fb 100644
> --- a/examples/bond/main.c
> +++ b/examples/bond/main.c
> @@ -96,13 +96,12 @@
>  		":%02"PRIx8":%02"PRIx8":%02"PRIx8,	\
>  		RTE_ETHER_ADDR_BYTES(&addr))
>  
> -uint16_t members[RTE_MAX_ETHPORTS];
> -uint16_t members_count;
> +static uint16_t members[RTE_MAX_ETHPORTS];
> +static uint16_t members_count;
> +static struct rte_mempool *mbuf_pool;

Good addition of static storage qualifiers. Does adding static here affect 
any code outside this file that might reference these globals?

>  static uint16_t BOND_PORT = 0xffff;
>  
> -static struct rte_mempool *mbuf_pool;

[ ... ]

> -static void
> -member_port_init(uint16_t portid, struct rte_mempool *mbuf_pool)
> +member_port_init(uint16_t portid)

Does removing the mbuf_pool parameter here create any issues with 
testability or future refactoring where different pools might be needed per 
port?

[ ... ]

> diff --git a/examples/dma/dmafwd.c b/examples/dma/dmafwd.c
> index 5ba0aaa40b..a8ed09e6ae 100644
> --- a/examples/dma/dmafwd.c
> +++ b/examples/dma/dmafwd.c
> @@ -590,11 +590,11 @@ dma_parse_portmask(const char *portmask)
>  }
>  
>  static copy_mode_t
> -dma_parse_copy_mode(const char *copy_mode)
> +dma_parse_copy_mode(const char *mode)
>  {
> -	if (strcmp(copy_mode, COPY_MODE_SW) == 0)
> +	if (strcmp(mode, COPY_MODE_SW) == 0)

Does this abbreviated parameter name maintain sufficient clarity given that 
the function is specifically about parsing copy modes?

[ ... ]

> @@ -1061,7 +1061,7 @@ main(int argc, char **argv)
>  	/* Initialize each port. 8< */
>  	cfg.nb_ports = 0;
>  	RTE_ETH_FOREACH_DEV(portid)
> -		port_init(portid, dma_pktmbuf_pool, nb_queues);
> +		port_init(portid, dma_pktmbuf_pool);

Does removing nb_queues parameter work correctly? Can port_init access 
nb_queues from global scope reliably?

[ ... ]

> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index a1a996dee8..866ba04b86 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -1828,8 +1827,8 @@ outbound_sa_lookup(struct sa_ctx *sa_ctx, uint32_t sa_idx[],
>   * Select HW offloads to be used.
>   */
>  int
> -sa_check_offloads(uint16_t port_id, uint64_t *rx_offloads,
> -		uint64_t *tx_offloads, uint8_t *hw_reassembly)
> +sa_check_offloads(uint16_t port_id, uint64_t *rx_flags,
> +		uint64_t *tx_flags, uint8_t *hw_reassembly)

Does changing parameter names from rx_offloads/tx_offloads to rx_flags/
tx_flags maintain consistency with DPDK naming conventions? The values are 
actually offload flags per rte_ethdev.h.

[ ... ]

> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 4c64194794..df035b508c 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -94,7 +94,7 @@ xmm_t val_eth[RTE_MAX_ETHPORTS];
>  uint32_t enabled_port_mask;
>  
>  /* Used only in exact match mode. */
> -int ipv6; /**< ipv6 is false by default. */
> +bool ipv6_enabled; /**< ipv6 is false by default. */

Good improvement changing int to bool for a boolean variable. Does this 
change affect any code that tests ipv6 with non-boolean comparisons?

[ ... ]

> diff --git a/examples/l3fwd/l3fwd_route.h b/examples/l3fwd/l3fwd_route.h
> index b02b9cc11c..fd688504c6 100644
> --- a/examples/l3fwd/l3fwd_route.h
> +++ b/examples/l3fwd/l3fwd_route.h
> @@ -14,14 +14,14 @@
>  #define	IPV6_ADDR_U32	(IPV6_ADDR_LEN / sizeof(uint32_t))
>  
>  #define GET_CB_FIELD(in, fd, base, lim, dlm)	do {            \
> -	unsigned long val;                                      \
> -	char *end;                                              \
> +	unsigned long _val;                                     \
> +	char *_end;                                             \

Good use of underscore prefix for macro-internal variables to avoid 
shadowing. Do these variable names follow DPDK macro naming conventions?