|WARNING| [26.11,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/166587

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None.

### Warnings

None.

### Info

The test additions look comprehensive and follow proper TEST_ASSERT patterns.
Sentinel verification using `0x5a` fill pattern is a good technique for detecting buffer overruns.

---

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

### Errors

**Resource leak on test failure** (line 210 in context)

```c
cfgfile = rte_cfgfile_create(0);
TEST_ASSERT_NOT_NULL(cfgfile, "Failed to create cfgfile");

ret = rte_cfgfile_add_section(cfgfile, "section1");
TEST_ASSERT_SUCCESS(ret, "Failed to add section1");
```

If any `TEST_ASSERT*` macro fails after `rte_cfgfile_create()` succeeds (lines 213, 215, 217, 220, 222, 224, 227, 230, 233, 236),
the function returns immediately without calling `rte_cfgfile_close(cfgfile)` (line 236).
The `cfgfile` resource leaks.

**Suggested fix:** Add a cleanup label and convert early-return assertions to use goto:

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

	cfgfile = rte_cfgfile_create(0);
	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to create cfgfile");

	ret = rte_cfgfile_add_section(cfgfile, "section1");
	if (ret != 0) {
		test_result = -1;
		goto cleanup_cfgfile;
	}
	/* ... similar pattern for other assertions up to save ... */

	ret = rte_cfgfile_save(cfgfile, filename);
	if (ret != 0) {
		test_result = -1;
		goto cleanup_cfgfile;
	}

cleanup_cfgfile:
	if (cfgfile != NULL) {
		ret = rte_cfgfile_close(cfgfile);
		if (ret != 0 && test_result == 0)
			test_result = ret;
	}
	if (test_result != 0)
		return test_result;

	loaded = rte_cfgfile_load(filename, 0);
	TEST_ASSERT_NOT_NULL(loaded, "Failed to load saved cfgfile");
	/* ... rest of test with similar pattern for loaded ... */
cleanup_loaded:
	if (loaded != NULL)
		rte_cfgfile_close(loaded);
	remove(filename);
	return test_result;
}
```

Note: The file `filename` is also not removed if any assertion after `make_tmp_file()` succeeds fails.
Consider adding `remove(filename)` to the cleanup path as well.

---

**Resource leak on test failure** (line 250 in context)

Same issue: if `rte_cfgfile_close(loaded)` at line 259 is not reached due to an earlier assertion failure (lines 242-257),
the `loaded` cfgfile leaks.

---

### Warnings

None.

---

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

### Errors

**Resource leak on test failure** (line 508 in context)

```c
cfgfile = rte_cfgfile_load(filename, 0);
TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");

ret = rte_cfgfile_has_entry(cfgfile, "section2", "key2");
TEST_ASSERT(ret == 1, "section2 key2 entry missing");
```

If any assertion from line 509 through line 530 fails,
the function returns immediately without calling `rte_cfgfile_close(cfgfile)` (line 530).
The `cfgfile` resource leaks.

**Suggested fix:** Use goto cleanup pattern as in Patch 2/4.

---

**Resource leak on test failure** (line 533 in context)

Same issue for `loaded`: if assertions at lines 535-537 fail,
`rte_cfgfile_close(loaded)` at line 540 is not called.

---

### Warnings

None.

---

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

### Errors

None.

### Warnings

None.

### Info

Good test addition.
The expanded line now properly exercises the line-length error path.

---

## Summary

**Critical findings:**
- **3 resource leaks** in test functions (Patch 2/4 has 2, Patch 3/4 has 2):
  `rte_cfgfile` handles are not closed when `TEST_ASSERT*` macros cause early return.
  All three test functions need refactoring to use a cleanup label pattern.

**Style/Process:**
- All patches follow TEST_ASSERT infrastructure correctly.
- Code style is consistent with DPDK conventions.
- No forbidden tokens detected.
- Test registration uses correct `TEST_CASE()` macros.

**Recommendation:**
Fix the resource leaks before merging.
These are test-only but should follow correct resource management patterns.