|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 6de0ea50e9b9 (example commit sha placeholder)
Author: Andrei-Niculae Petre <p31andrei at gmail.com>
examples/l3fwd: fix compilation when DP debug on

Previously, l3fwd example app would fail to compile if one would put
RTE_LOG_DP_LEVEL=RTE_LOG_DEBUG in config/rte_config.h and try to
compile dpdk; this path was not enabled via compiling debug build
(--buildtype=debug) alone, but needed the data path to be at debug
level.

> diff --git a/.mailmap b/.mailmap
> index cdaa471715..df999d00d8 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -106,6 +106,7 @@ Andre Muezerie <andremue at linux.microsoft.com> <andremue at microsoft.com>
>  Andre Richter <andre.o.richter at gmail.com>
>  Andrea Arcangeli <aarcange at redhat.com>
>  Andrea Grandi <andrea.grandi at intel.com>
> +Andrei-Niculae Petre <p31andrei at gmail.com>

[ ... ]

> 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"
> +

Is it intentional to include the header file so early in the .c file?
This creates an unusual ordering where the header is included before the
static data structures (acl_config, etc.) that use types potentially
defined later.  Does this potentially violate any forward declaration
requirements or create subtle ordering dependencies?

>  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"
> -
>  #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;

[ ... ]

> @@ -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];

Good catch on using the correct constant for IPv4.

>  	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,
>  		abuf, sizeof(abuf)));

Does the API of inet_ntop() require a pointer argument?  The
rte_ipv4_hdr structure defines src_addr and dst_addr as rte_be32_t
(which is uint32_t), not as arrays.  Does passing their addresses
(&ipv4_hdr->src_addr) to inet_ntop() match the expected interface?

>  
>  	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)));

Does this change make sense for IPv6 addresses?  The rte_ipv6_hdr
structure defines src_addr and dst_addr as uint8_t arrays of size 16.
Was the original code passing the array directly to inet_ntop() correct,
and does adding the address-of operator (&) now pass a pointer to the
array instead of the array itself?

>  
>  	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);

Good fix on removing the incorrect array indexing.

[ ... ]

> 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 */