Re: |WARNING| argparse: support range step for corelist parsing

Re: |WARNING| argparse: support range step for corelist parsing

[fengchengwen]

On 10/16/2026 6:09 PM, dpdklab@iol.unh.edu wrote:
> Test-Label: ai-code-review
> Test-Status: WARNING
> https://dpdk.org/patch/171477
> 
> _AI Code Review_
> 
> === Patch Review: series.patch (via Claude) ===
> Review date: 2026-10-16
> 
> # DPDK Patch Review
> 
> ## Patch: argparse: support range step for corelist parsing
> 
> This patch adds stride/step syntax (`start-end:step`) to the EAL corelist parser to help users skip sibling cores more easily (e.g., `-l 0-126:2` instead of `-l 0,2,4,6...126`).
> 
> ---
> 
> ## ERRORS
> 
> None identified.
> 
> ---
> 
> ## WARNINGS
> 
> ### 1. Missing release notes update
> 
> The patch adds new API functionality (range step syntax in corelist parsing) but does not update the release notes.
> 
> **Location:** No changes to `doc/guides/rel_notes/release_*.rst`
> 
> **Why it matters:** User-visible feature additions require release notes per DPDK guidelines.
> 
> **Suggested fix:** Add a feature note in the current release notes file under the appropriate section:
> 
> ```rst
> * **argparse: Added step syntax for corelist ranges.**
> 
>   The corelist parser now supports stride/step syntax for core ranges.
>   For example, ``-l 0-126:2`` selects cores 0, 2, 4, ..., 126.
>   This is useful for skipping sibling cores on systems with SMT/hyperthreading.

OK, will done in v2

> ```
> 
> ---
> 
> ### 2. Test coverage: missing zero-step test case
> 
> The patch adds validation that step must be positive (`if (idx == 0)`) but does not include a test case for step=0.
> 
> **Location:** `app/test/test_argparse.c` line 605-613 validates step > 0, but no test exercises `"1-5:0"`.
> 
> **Why it matters:** Untested error paths may hide bugs. The zero-step case is explicitly rejected in code but not verified in the test suite.
> 
> **Suggested fix:** Add a test case:
> 
> ```c
> char *corelist_invalid_range_step_zero = test_strdup("1-5:0");
> /* ... later in test ... */
> CPU_ZERO(&val_cpuset);
> ret = rte_argparse_parse_type(corelist_invalid_range_step_zero,
>         RTE_ARGPARSE_VALUE_TYPE_CORELIST, &val_cpuset);
> TEST_ASSERT(ret != 0, "Argparse parse type for corelist (step zero) should have failed!");
> ```

OK, will done in v2

> 
> ---
> 
> ## INFO
> 
> ### 1. Code clarity: `max` variable initialization
> 
> The patch changes `max` from a local variable declared inside the loop (`int32_t max;`) to a variable declared at function scope and initialized to `-1`.
> 
> **Location:** `lib/argparse/rte_argparse.c:578` and line 590 (removed local declaration)
> 
> **Observation:** This is correct. The variable needs to persist across loop iterations for the new step logic. The change does not introduce a bug and improves correctness.
> 
> ---
> 
> ### 2. Documentation matches implementation
> 
> The documentation in `doc/guides/prog_guide/argparse_lib.rst` accurately describes the new step syntax with examples. Cross-checked against the implementation logic.
> 
> ---
> 
> ### 3. Test coverage is comprehensive
> 
> The test suite covers:
> - Valid step syntax: `1-5:2`, mixed with commas, reverse ranges with step
> - Invalid cases: incomplete step syntax, double colons, negative step, extra colons
> - Edge cases: step larger than range size (e.g., `12-16:3` correctly sets 12, 15)
> 
> The tests verify both that cores in range are set and cores outside range are not set, which is thorough.
> 
> ---
> 
> ### 4. Variable naming: `is_step` boolean
> 
> The patch introduces `bool is_step` at line 576. This follows DPDK guidelines for boolean variable naming (using `bool` from `<stdbool.h>`).

The original rte_argparse library already include <stdbool.h> in rte_argparse.h

> 
> ---
> 
> Review-Result: WARNING

Re: |WARNING| argparse: support range step for corelist parsing

[fengchengwen]

On 10/16/2026 6:12 PM, dpdklab@iol.unh.edu wrote:
> Test-Label: ai-code-review
> Test-Status: WARNING
> https://dpdk.org/patch/171477
> 
> _AI Code Review_
> 
> === Patch Review: series.patch (via Claude) ===
> Review date: 2026-10-16
> 
> # DPDK Patch Review
> 
> ## Patch: argparse: support range step for corelist parsing
> 
> ### ERRORS
> 
> **Error 1: Integer overflow in step accumulation (correctness bug)**
> 
> Line 629 in `lib/argparse/rte_argparse.c`:
> ```c
> for (; min <= max; min += step)
> ```
> 
> When `step` is large and `min + step` exceeds `INT32_MAX`, the loop counter wraps around to a negative value, creating an infinite loop. The variables `min` and `max` are `int` (32-bit signed), and `step` is `int32_t`. If a malicious or mistaken user provides input like `"0-100:2147483647"`, the addition `min += step` will overflow after the first iteration.

False positive, the:
		errno = 0;
		idx = strtol(value, &end, 10);
		last = end;
		if (errno || idx > UINT16_MAX) {
			ARGPARSE_LOG(ERR, "argument %s contains a numerical value out of range!",
				arg->name_long);
			return -EINVAL;
		}
already treat such input as an error.

> 
> **Suggested fix:**
> Check that `step` does not cause overflow before the loop:
> ```c
> if (step > max - min) {
>     ARGPARSE_LOG(ERR, "argument %s step value too large for range!",
>         arg->name_long);
>     return -EINVAL;
> }
> for (; min <= max; min += step)
>     CPU_SET(min, cpuset);
> ```
> 
> ---
> 
> **Error 2: Step validation insufficient (correctness bug)**
> 
> Lines 608-613 in `lib/argparse/rte_argparse.c`:
> ```c
> if (is_step) {
>     if (idx == 0) {
>         ARGPARSE_LOG(ERR, "argument %s step value must be positive!",
>             arg->name_long);
>         return -EINVAL;
>     }
>     step = idx;
> }
> ```
> 
> This only rejects zero but allows negative values. A negative step (e.g., `"1-5:-2"`) would pass this check because `idx` is `int64_t` and `-2 != 0`. The loop at line 629 would then decrement `min` instead of incrementing it, either becoming infinite or setting unintended cores.

False positive, the:
		if (!isdigit(*value)) {
			ARGPARSE_LOG(ERR, "argument %s has an unexpected non digit character!",
				arg->name_long);
			return -EINVAL;
		}
already treat -2 as an error.

> 
> **Suggested fix:**
> ```c
> if (is_step) {
>     if (idx <= 0) {
>         ARGPARSE_LOG(ERR, "argument %s step value must be positive!",
>             arg->name_long);
>         return -EINVAL;
>     }
>     step = idx;
> }
> ```
> 
> ---
> 
> **Error 3: Step value not clamped to 32-bit range before assignment (correctness bug)**
> 
> Line 613 in `lib/argparse/rte_argparse.c`:
> ```c
> step = idx;
> ```
> 
> `idx` is `int64_t` (from `rte_strtoint64_l()`), but `step` is `int32_t`. If the user provides `"0-10:4294967296"` (2^32), the assignment truncates the upper 32 bits, and `step` becomes 0, causing an infinite loop at line 629 or violating the earlier zero-check if the truncated value is negative.

False positive, the:
		if (!isdigit(*value)) {
			ARGPARSE_LOG(ERR, "argument %s has an unexpected non digit character!",
				arg->name_long);
			return -EINVAL;
		}

		errno = 0;
		idx = strtol(value, &end, 10);
		last = end;
		if (errno || idx > UINT16_MAX) {
			ARGPARSE_LOG(ERR, "argument %s contains a numerical value out of range!",
				arg->name_long);
			return -EINVAL;
		}

it mean min/max/step must be in [0, UINT16_MAX] after above verify

> 
> **Suggested fix:**
> Add a range check after the positive validation:
> ```c
> if (is_step) {
>     if (idx <= 0) {
>         ARGPARSE_LOG(ERR, "argument %s step value must be positive!",
>             arg->name_long);
>         return -EINVAL;
>     }
>     if (idx > INT32_MAX) {
>         ARGPARSE_LOG(ERR, "argument %s step value exceeds maximum!",
>             arg->name_long);
>         return -EINVAL;
>     }
>     step = (int32_t)idx;
> }
> ```
> 
> ---
> 
> **Error 4: `max` and `min` signed/unsigned type confusion (correctness bug)**
> 
> Lines 576-579 in `lib/argparse/rte_argparse.c`:
> ```c
> int32_t step = 1;
> int32_t max = -1;
> int min = -1;
> ```
> 
> `min` is declared as `int` (typically 32-bit signed), while `max` is `int32_t` (explicitly 32-bit signed). This inconsistency is a latent bug: if `int` is 16-bit on some platform, `min` and `max` have different ranges. Modern DPDK targets assume 32-bit `int`, but the inconsistency is unnecessary and confusing.

False positive, DPDK only support at least 32bit platform, the int is 32-bit at such platform.

> 
> **Suggested fix:**
> Declare `min` as `int32_t` for consistency:
> ```c
> int32_t step = 1;
> int32_t max = -1;
> int32_t min = -1;
> ```
> 
> ---
> 
> ### WARNINGS
> 
> **Warning 1: Release notes missing for new user-visible feature**
> 
> The patch adds a new user-facing syntax (`start-end:step`) to the `-l` corelist parser, which is a significant functional enhancement. The documentation in `doc/guides/prog_guide/argparse_lib.rst` is updated, but there is no corresponding entry in the release notes (`doc/guides/rel_notes/release_*.rst`) announcing this feature.
> 
> **Suggested fix:**
> Add a release notes entry under "New Features" in the current release notes file describing the new range step syntax for corelist parsing.

OK, will done in v2

> 
> ---
> 
> **Warning 2: Test coverage for boundary conditions incomplete**
> 
> The test suite in `app/test/test_argparse.c` adds extensive tests for the step syntax, including invalid cases. However, it does not test:
> - Maximum representable step values near `INT32_MAX` (boundary test)
> - Step values that would cause integer overflow in the loop counter
> - Step values larger than the range itself (e.g., `"0-5:10"`, which should set only core 0)
> - Very large core numbers near `CPU_SETSIZE` (e.g., `"1000-1010:2"` if supported)

OK, will add some of them

> 
> **Suggested fix:**
> Add tests for these boundary conditions to ensure the overflow checks (once added per Error 1) work correctly and that the implementation behaves as expected in edge cases.
> 
> ---
> 
> ### INFO
> 
> None.
> 
> ---
> 
> ## Summary
> 
> The patch introduces useful functionality but has four correctness bugs that must be fixed:
> 1. Integer overflow in the step loop can cause infinite loops
> 2. Negative step values are not rejected
> 3. 64-to-32-bit step truncation is unchecked, allowing zero/negative truncated values
> 4. Inconsistent `int` vs `int32_t` declaration for `min` and `max`
> 
> Additionally, release notes are missing for this user-visible feature change, and test coverage could be improved for boundary conditions.
> 
> ---
> 
> Review-Result: ERROR

Re: |WARNING| argparse: support range step for corelist parsing

[Stephen Hemminger]

On Mon, 19 Oct 2026 16:36:12 +0800
fengchengwen <fengchengwen@huawei.com> wrote:

> > When `step` is large and `min + step` exceeds `INT32_MAX`, the loop counter wraps around to a negative value, creating an infinite loop. The variables `min` and `max` are `int` (32-bit signed), and `step` is `int32_t`. If a malicious or mistaken user provides input like `"0-100:2147483647"`, the addition `min += step` will overflow after the first iteration.  
> 
> False positive, the:
> 		errno = 0;
> 		idx = strtol(value, &end, 10);
> 		last = end;
> 		if (errno || idx > UINT16_MAX) {
> 			ARGPARSE_LOG(ERR, "argument %s contains a numerical value out of range!",
> 				arg->name_long);
> 			return -EINVAL;
> 		}
> already treat such input as an error.

Since non-negative steps don't make sense, I would use strtoul() instead
and bound it by MAX_LCORE