|WARNING| [v2,4/4] test/cfgfile: test for long lines in file

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/167875

_AI Code Review_

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

# Review of DPDK Patch Series

## Summary
This is a 4-patch test-only series that improves test coverage for the cfgfile library. All patches are test additions/improvements with no library code changes. Overall the series is well-structured and correct.

---

## Patch 1/4: test/cfgfile: improve coverage for listing APIs

**No issues found.**

The patch adds comprehensive test coverage for section/entry listing APIs with proper sentinel validation to ensure array bounds are respected.

---

## Patch 2/4: test/cfgfile: validate config creation APIs

### Errors

**Resource leak on error path:**
In `test_cfgfile_create_add_save_reload()`, if `rte_cfgfile_save()` fails (line 229), the function returns without closing `cfgfile`:

```c
ret = rte_cfgfile_save(cfgfile, filename);
TEST_ASSERT_SUCCESS(ret, "Failed to save cfgfile");  // returns here on failure

ret = rte_cfgfile_close(cfgfile);  // never reached if save fails
```

**Fix:** Add cleanup on all error paths after `cfgfile` is created:

```c
ret = rte_cfgfile_save(cfgfile, filename);
if (ret != 0) {
    rte_cfgfile_close(cfgfile);
    remove(filename);
    TEST_ASSERT_SUCCESS(ret, "Failed to save cfgfile");
}
```

Same pattern applies to the `rte_cfgfile_load()` failure path at line 236 -- the temporary file is not removed on failure.

---

## Patch 3/4: test/cfgfile: verify file modification API

### Errors

**Resource leak on error path (multiple instances):**
In `test_cfgfile_modify_entry()`, several error paths fail to clean up:

1. If `rte_cfgfile_load()` fails (line 507), `filename` is not removed
2. If `rte_cfgfile_has_entry()` fails (line 509-512), `cfgfile` is not closed and `filename` is not removed
3. If `rte_cfgfile_set_entry()` fails (line 515), same leak
4. If the negative tests fail (lines 518-524), same leak
5. If `rte_cfgfile_save()` fails (line 526), `cfgfile` is not closed
6. If `rte_cfgfile_close()` fails (line 529), `filename` is not removed
7. If the second `rte_cfgfile_load()` fails (line 532), `filename` is not removed

**Fix:** Use a cleanup label pattern:

```c
test_cfgfile_modify_entry(void)
{
    struct rte_cfgfile *cfgfile = NULL;
    struct rte_cfgfile *loaded = NULL;
    char filename[PATH_MAX];
    int ret;
    int test_result = 0;

    ret = make_tmp_file(filename, "sample1_set", sample1_ini);
    TEST_ASSERT_SUCCESS(ret, "Failed to setup temp file");

    cfgfile = rte_cfgfile_load(filename, 0);
    if (cfgfile == NULL) {
        test_result = -1;
        goto cleanup;
    }

    /* ... tests ... */

    ret = rte_cfgfile_close(loaded);
    if (ret != 0) {
        test_result = ret;
        goto cleanup;
    }

cleanup:
    if (cfgfile != NULL)
        rte_cfgfile_close(cfgfile);
    if (loaded != NULL)
        rte_cfgfile_close(loaded);
    remove(filename);
    
    TEST_ASSERT_SUCCESS(test_result, "Test failed");
    return test_result;
}
```

---

## Patch 4/4: test/cfgfile: test for long lines in file

### Errors

**Resource leak on error path:**
In `test_cfgfile_line_too_long()`, if `rte_cfgfile_load()` unexpectedly succeeds (line 389), the test asserts failure but does not close `cfgfile` before returning:

```c
cfgfile = rte_cfgfile_load(filename, 0);
TEST_ASSERT_NULL(cfgfile, "Expected failure did not occur");  // leaks cfgfile if non-NULL
```

**Fix:**

```c
cfgfile = rte_cfgfile_load(filename, 0);
if (cfgfile != NULL) {
    rte_cfgfile_close(cfgfile);
    remove(filename);
    TEST_ASSERT_NULL(cfgfile, "Expected failure did not occur");
}
```

---

## General Observations

All patches are test-only changes and do not require release notes (per guidelines, test-only changes are excluded).

The functional test infrastructure properly uses `TEST_ASSERT` macros and is registered with `TEST_CASE` as required.

No style, API, or documentation issues found -- the code follows DPDK coding conventions correctly.

---

## Summary of Required Changes

**All errors are resource leaks on error paths.** Each test function allocates resources (cfgfile handles, temporary files) and has error paths that return early via `TEST_ASSERT_*` macros without cleanup. The TEST_ASSERT macros expand to early returns, so any resource allocated before a failed assertion leaks.

**Recommendation:** Restructure all new test functions to use a cleanup-label pattern or ensure all error paths explicitly free resources before calling TEST_ASSERT.