|SUCCESS| examples/l3fwd: fix compilation when DP debug on

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Commit: [Unable to determine - reviewing patch file]
Author: Andrei-Niculae Petre <p31andrei at gmail.com>
Subject: [PATCH] examples/l3fwd: fix compilation when DP debug on

This patch fixes a compilation failure in the l3fwd example when
RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. The changes include moving
header includes, changing function visibility, and fixing type mismatches
in debug code.

> diff --git a/.mailmap b/.mailmap
> index cdaa471715..df999d00d8 100644
> --- a/.mailmap
> +++ b/.mailmap

[ ... ]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5683b87e4a..501f312ba1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS

[ ... ]

> diff --git a/examples/l3fwd/l3fwd_acl.c b/examples/l3fwd/l3fwd_acl.c
> index a1275e18bc..8e24f1c599 100644
> --- a/examples/l3fwd/l3fwd_acl.c
> +++ b/examples/l3fwd/l3fwd_acl.c
> @@ -235,6 +235,9 @@ enum {
>  RTE_ACL_RULE_DEF(acl4_rule, RTE_DIM(ipv4_defs));
>  RTE_ACL_RULE_DEF(acl6_rule, RTE_DIM(ipv6_defs));
>  
> +/* Must be included before acl_config definition to enable L3FWDACL_DEBUG */
> +#include "l3fwd_acl.h"
> +
>  static struct {
>  	struct rte_acl_ctx *acx_ipv4[NB_SOCKETS];
>  	struct rte_acl_ctx *acx_ipv6[NB_SOCKETS];
> @@ -251,8 +254,6 @@ static struct rte_acl_rule *acl_base_ipv4, *route_base_ipv4,
>  static unsigned int acl_num_ipv4, route_num_ipv4,
>  		acl_num_ipv6, route_num_ipv6;
>  
> -#include "l3fwd_acl.h"
> -

Does this header reordering create any issues with structure definitions?
The header is now included before acl_base_ipv4, route_base_ipv4, and other
static variables are declared. Does the header depend on any of these
declarations, or vice versa?

>  #include "l3fwd_acl_scalar.h"
>  
>  /*
> @@ -773,7 +774,7 @@ read_config_files_acl(void)
>  	}
>  }
>  
> -void
> +static void
>  print_one_ipv4_rule(struct acl4_rule *rule, int extra)
>  {
>  	char abuf[INET6_ADDRSTRLEN];
> @@ -800,7 +801,7 @@ print_one_ipv4_rule(struct acl4_rule *rule, int extra)
>  			rule->data.userdata);
>  }
>  
> -void
> +static void
>  print_one_ipv6_rule(struct acl6_rule *rule, int extra)
>  {
>  	unsigned char a, b, c, d;

The print_one_ipv4_rule() and print_one_ipv6_rule() functions are being
changed from external to static linkage. Were these functions previously
exported in the header file? If so, are there any other compilation units
that might be calling these functions?

> @@ -855,17 +856,17 @@ print_one_ipv6_rule(struct acl6_rule *rule, int extra)
>  
>  #ifdef L3FWDACL_DEBUG
>  static inline void
> -dump_acl4_rule(struct rte_mbuf *m, uint32_t sig)
> +dump_acl4_rule(const struct rte_mbuf *m, uint32_t sig)
>  {
> -	char abuf[INET6_ADDRSTRLEN];
> +	char abuf[INET_ADDRSTRLEN];

This buffer size change from INET6_ADDRSTRLEN to INET_ADDRSTRLEN is
correct for IPv4 addresses. Does inet_ntop() safely handle the smaller
buffer when formatting IPv4 addresses?

>  	uint32_t offset = sig & ~ACL_DENY_SIGNATURE;
>  	struct rte_ipv4_hdr *ipv4_hdr =
>  		rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
>  					sizeof(struct rte_ether_hdr));
>  
> -	printf("Packet Src:%s ", inet_ntop(AF_INET, ipv4_hdr->src_addr,
> +	printf("Packet Src:%s ", inet_ntop(AF_INET, &ipv4_hdr->src_addr,
>  		abuf, sizeof(abuf)));
> -	printf("Dst:%s ", inet_ntop(AF_INET, ipv4_hdr->dst_addr,
> +	printf("Dst:%s ", inet_ntop(AF_INET, &ipv4_hdr->dst_addr,

The addition of address-of operators here suggests the original code had
type mismatches. Does inet_ntop() expect a pointer to the address, and
were src_addr and dst_addr being passed incorrectly before?

>  		abuf, sizeof(abuf)));
>  
>  	printf("Src port:%hu,Dst port:%hu ",
> @@ -879,7 +880,7 @@ dump_acl4_rule(struct rte_mbuf *m, uint32_t sig)
>  }
>  
>  static inline void
> -dump_acl6_rule(struct rte_mbuf *m, uint32_t sig)
> +dump_acl6_rule(const struct rte_mbuf *m, uint32_t sig)
>  {
>  	char abuf[INET6_ADDRSTRLEN];
>  	uint32_t offset = sig & ~ACL_DENY_SIGNATURE;
> @@ -888,10 +889,10 @@ dump_acl6_rule(struct rte_mbuf *m, uint32_t sig)
>  					sizeof(struct rte_ether_hdr));
>  
>  	printf("Packet Src");
> -	printf("%s", inet_ntop(AF_INET6, ipv6_hdr->src_addr,
> +	printf("%s", inet_ntop(AF_INET6, &ipv6_hdr->src_addr,
>  		abuf, sizeof(abuf)));
>  	printf("\nDst");
> -	printf("%s", inet_ntop(AF_INET6, ipv6_hdr->dst_addr,
> +	printf("%s", inet_ntop(AF_INET6, &ipv6_hdr->dst_addr,
>  		abuf, sizeof(abuf)));
>  
>  	printf("\nSrc port:%hu,Dst port:%hu ",
> @@ -984,8 +985,8 @@ dump_denied_pkt(const struct rte_mbuf *pkt, uint32_t res)
>  	if ((res & ACL_DENY_SIGNATURE) != 0) {
>  		if (RTE_ETH_IS_IPV4_HDR(pkt->packet_type))
>  			dump_acl4_rule(pkt, res);
> -		else if (RTE_ETH_IS_IPV6_HDR(pkt[i]->packet_type))
> -			dump_acl6_rule(pkt[i], res[i]);
> +		else if (RTE_ETH_IS_IPV6_HDR(pkt->packet_type))
> +			dump_acl6_rule(pkt, res);

This looks like a significant bug fix. The original code references
pkt[i] and res[i], but there is no visible loop variable i in this
context. Does the function dump_denied_pkt() take a single packet pointer
rather than an array? Was this code ever tested with L3FWDACL_DEBUG
enabled?

>  	}
>  #else
>  	RTE_SET_USED(pkt);
> diff --git a/examples/l3fwd/l3fwd_acl.h b/examples/l3fwd/l3fwd_acl.h
> index 05af62fe10..a34a08eb7f 100644
> --- a/examples/l3fwd/l3fwd_acl.h
> +++ b/examples/l3fwd/l3fwd_acl.h
> @@ -48,10 +48,4 @@
>   */
>  #define FWD_PORT_SHIFT 1
>  
> -void
> -print_one_ipv4_rule(struct acl4_rule *rule, int extra);
> -
> -void
> -print_one_ipv6_rule(struct acl6_rule *rule, int extra);
> -
>  #endif /* L3FWD_ACL_H */

The removal of these function declarations matches the change to static
linkage in the .c file. Are these functions still used anywhere, or were
they only used internally within l3fwd_acl.c?