|SUCCESS| [RFC, 3/3] test/flow_compile: add unit tests for flow rule compiler

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# Review of flow_compile library patch

## Commit Information

git sha: Not provided in patch format (patchwork submission)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [RFC 1/3] flow_compile: introduce textual flow rule compiler

This patch introduces a new librte_flow_compile library that compiles
textual flow rule descriptions into rte_flow_attr/item/action arrays.
The parser is table-driven and supports common flow items and actions
with a grammar similar to testpmd.

## Review Comments

### Resource Management Issues

> diff --git a/lib/flow_compile/flow_compile_parse.c b/lib/flow_compile/flow_compile_parse.c
> new file mode 100644
> index 0000000000..d39b7ef5a5
> --- /dev/null
> +++ b/lib/flow_compile/flow_compile_parse.c

[ ... ]

> +static int
> +parse_item_fields(struct flow_compile_ctx *cc,
> +		  const struct flow_item_desc *desc,
> +		  struct rte_flow_item *item)
> +{
> +	if (desc->spec_size > 0) {
> +		item->spec = rte_zmalloc("flow_compile", desc->spec_size, 0);
> +		item->mask = rte_zmalloc("flow_compile", desc->spec_size, 0);
> +		item->last = rte_zmalloc("flow_compile", desc->spec_size, 0);
> +		if (item->spec == NULL || item->mask == NULL ||
> +		    item->last == NULL) {
> +			rte_errno = ENOMEM;
> +			return -1;
> +		}
> +	}

Does this code leak memory when only some allocations succeed? If
item->spec succeeds but item->mask fails, does the cleanup path properly
free item->spec?

> +	/* Drop unused buffers; the PMD treats NULL as default. */
> +	if (!spec_used) {
> +		rte_free(spec);
> +		item->spec = NULL;
> +	}
> +	if (!mask_used) {
> +		rte_free(mask);
> +		item->mask = NULL;
> +	}
> +	if (!last_used) {
> +		rte_free(last);
> +		item->last = NULL;
> +	}
> +	return 0;
> +}

The cleanup at the end looks correct for the success path, but what about
the error paths between allocation and this cleanup?

[ ... ]

> +static int
> +parse_pattern(struct flow_compile_ctx *cc, struct rte_flow_compile *out)
> +{
> +	if (!tok_is_ident(&cc->tok, "pattern"))
> +		return flow_compile_errf(cc, &cc->tok, "expected 'pattern'");
> +	if (flow_compile_lex(cc) < 0)
> +		return -1;
> +
> +	size_t cap = 8;
> +	out->pattern = rte_calloc("flow_compile_pattern", cap,
> +				  sizeof(*out->pattern), 0);
> +	if (out->pattern == NULL) {
> +		rte_errno = ENOMEM;
> +		return -1;
> +	}

> +		if (out->npattern + 1 >= cap) {
> +			cap *= 2;
> +			struct rte_flow_item *p =
> +				rte_realloc(out->pattern,
> +					    cap * sizeof(*p), 0);
> +			if (p == NULL) {
> +				rte_errno = ENOMEM;
> +				return -1;
> +			}
> +			out->pattern = p;
> +		}

Does rte_realloc() preserve the original pointer if it fails? If the
realloc fails, can the cleanup code still reach the original out->pattern
allocation?

### Coding Style Issues

> diff --git a/lib/flow_compile/flow_compile_lex.c b/lib/flow_compile/flow_compile_lex.c
> new file mode 100644
> index 0000000000..f58de29415
> --- /dev/null
> +++ b/lib/flow_compile/flow_compile_lex.c

[ ... ]

> +static inline bool
> +is_ascii_space(int c)
> +{
> +	return c == ' ' || c == '\t' || c == '\n' ||
> +	       c == '\r' || c == '\v' || c == '\f';
> +}

According to DPDK coding style, continuation lines should be indented to
align with the opening parenthesis. Does this alignment match the style
guide?

> +static bool
> +match_ipv4(const char *s, size_t n, uint8_t out[4])
> +{
> +	int parts = 0;
> +	uint32_t v = 0;
> +	size_t i = 0;
> +	bool in_part = false;

Should variable declarations follow the style of one declaration per line
per the coding style guide?

> +	while (i < n) {
> +		int c = (unsigned char)s[i];
> +		if (is_dec_digit(c)) {

Does this code correctly handle the case where an IPv4 octet exceeds 255?
The check happens later, but is there a risk of integer overflow in the v
accumulator before that check?

[ ... ]

> +static bool
> +match_ipv6(const char *s, size_t n, uint8_t out[16])
> +{
> +	uint16_t head[8] = {0};
> +	uint16_t tail[8] = {0};
> +	int nh = 0, nt = 0;
> +	bool seen_dcolon = false;
> +	size_t i = 0;

> +		if (i < n && s[i] == ':') {
> +			if (seen_dcolon)
> +				return false;
> +			seen_dcolon = true;
> +			i++;
> +			if (i == n)
> +				break;
> +		}

Does this code correctly reject malformed IPv6 addresses with multiple "::"
sequences?

### Portability Concerns

> diff --git a/lib/flow_compile/flow_compile_parse.c b/lib/flow_compile/flow_compile_parse.c

[ ... ]

> +static int
> +write_uint(struct flow_compile_ctx *cc, void *spec, void *mask_or_null,
> +	   const struct field_desc *fd, uint64_t v, uint64_t maxv,
> +	   const struct token *at)
> +{
> +	if (v > maxv)
> +		return flow_compile_errf(cc, at,
> +			"value %" PRIu64 " out of range for field '%s'",
> +			v, fd->name);
> +
> +	uint8_t *sp = (uint8_t *)spec + fd->offset;
> +	switch (fd->kind) {
> +	case FK_U8:
> +		*sp = (uint8_t)v;
> +		break;
> +	case FK_U16: {
> +		uint16_t x = (uint16_t)v;
> +		memcpy(sp, &x, sizeof(x));
> +		break;
> +	}

The code uses memcpy for potentially unaligned accesses, which is good.
Does this approach work correctly on both little-endian and big-endian
architectures given that FK_U16 is described as host order?

### Integer Overflow Concerns

> +static int
> +parse_uint(const char *s, size_t n, uint64_t *out)
> +{
> +	uint64_t v = 0;
> +	size_t i = 0;
> +	int base = 10;
> +
> +	if (n >= 2 && s[0] == '0' && (s[1] == 'x' || s[1] == 'X')) {
> +		base = 16;
> +		i = 2;
> +		if (i == n)
> +			return -1;
> +	}
> +	for (; i < n; i++) {
> +		uint64_t d;
> +		int c = (unsigned char)s[i];
> +		if (c >= '0' && c <= '9')
> +			d = (uint64_t)(c - '0');
> +		else if (base == 16 && c >= 'a' && c <= 'f')
> +			d = (uint64_t)(c - 'a' + 10);
> +		else if (base == 16 && c >= 'A' && c <= 'F')
> +			d = (uint64_t)(c - 'A' + 10);
> +		else
> +			return -1;
> +		/* overflow check */
> +		if (v > (UINT64_MAX - d) / (uint64_t)base)
> +			return -1;
> +		v = v * (uint64_t)base + d;
> +	}

Does this overflow check correctly handle all edge cases? What happens when
v is exactly at the boundary value before the multiplication?

### Error Handling

> diff --git a/lib/flow_compile/rte_flow_compile_api.c b/lib/flow_compile/rte_flow_compile_api.c
> new file mode 100644
> index 0000000000..92a80d4a24
> --- /dev/null
> +++ b/lib/flow_compile/rte_flow_compile_api.c

[ ... ]

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_flow_compile, 26.07)
> +struct rte_flow_compile *
> +rte_flow_compile(const char *str, char *errbuf)
> +{
> +	if (str == NULL || errbuf == NULL) {
> +		rte_errno = EINVAL;
> +		return NULL;
> +	}
> +	errbuf[0] = '\0';

Does this function correctly handle the case where str points to an empty
string versus a NULL pointer? Both are checked but have different semantics.

> +	struct flow_compile_ctx cc = {
> +		.src = str,
> +		.cur = str,
> +		.line = 1,
> +		.col  = 1,
> +		.errbuf = errbuf,
> +		.out = out,
> +	};
> +
> +	if (flow_compile_parse(&cc, out) < 0) {
> +		rte_flow_compile_free(out);
> +		return NULL;
> +	}

Does rte_flow_compile_free() safely handle a partially initialized out
structure if flow_compile_parse() fails early?

### Const Correctness

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_flow_compile_free, 26.07)
> +void
> +rte_flow_compile_free(struct rte_flow_compile *fc)
> +{
> +	if (fc == NULL)
> +		return;
> +	if (fc->pattern != NULL) {
> +		for (unsigned int i = 0; i < fc->npattern; i++) {
> +			/* Cast through uintptr_t to drop the API's
> +			 * const without -Wcast-qual; the parser owns
> +			 * these allocations.
> +			 */
> +			rte_free((void *)(uintptr_t)fc->pattern[i].spec);
> +			rte_free((void *)(uintptr_t)fc->pattern[i].mask);
> +			rte_free((void *)(uintptr_t)fc->pattern[i].last);
> +		}

This const-cast pattern seems intentional and documented. Does this approach
align with DPDK conventions for handling const pointers in cleanup code?

### Bounds Checking

> diff --git a/lib/flow_compile/flow_compile_lex.c b/lib/flow_compile/flow_compile_lex.c

[ ... ]

> +static bool
> +match_mac(const char *s, size_t n, uint8_t out[6])
> +{
> +	/* The longest accepted form is the 17-byte colon notation;
> +	 * Cisco notation is 14 bytes.  Cap at 18 to leave room for the
> +	 * NUL that rte_ether_unformat_addr() requires.
> +	 */
> +	char buf[18];
> +	if (n >= sizeof(buf))
> +		return false;
> +	memcpy(buf, s, n);
> +	buf[n] = '\0';

Does this code guarantee that n is always less than sizeof(buf) before the
memcpy, preventing buffer overflow?