|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

# DPDK Patch Review

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

### Errors

None.

### Warnings

None.

### Info

This patch adds comprehensive test coverage for listing APIs. The code is well-structured and follows DPDK test patterns correctly. The sentinel value checks (0x5a pattern) are a good defensive testing practice to verify array bounds are respected.

---

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

### Errors

**Resource leak on error path (line ~210):**
The `cfgfile` is created but not freed on the error paths before the first save.
If any of the `rte_cfgfile_add_section()` or `rte_cfgfile_add_entry()` calls fail, the function returns early via `TEST_ASSERT_SUCCESS()` without closing `cfgfile`, leaking the allocated structure.

**Suggested fix:**
```c
cfgfile = rte_cfgfile_create(0);
TEST_ASSERT_NOT_NULL(cfgfile, "Failed to create cfgfile");

ret = rte_cfgfile_add_section(cfgfile, "section1");
if (ret != 0) {
	rte_cfgfile_close(cfgfile);
	TEST_ASSERT_SUCCESS(ret, "Failed to add section1");
}
ret = rte_cfgfile_add_entry(cfgfile, "section1", "key1", "value1");
if (ret != 0) {
	rte_cfgfile_close(cfgfile);
	TEST_ASSERT_SUCCESS(ret, "Failed to add section1 key1");
}
/* ... similar for other add operations ... */
```

Or restructure to use a cleanup label:
```c
ret = rte_cfgfile_add_section(cfgfile, "section1");
if (ret != 0)
	goto cleanup;
/* ... */
cleanup:
	rte_cfgfile_close(cfgfile);
	TEST_ASSERT_SUCCESS(ret, "Failed operation");
```

**Resource leak on error path (line ~236):**
Similarly, the `loaded` cfgfile is not closed if any of the validation assertions fail after it is loaded.
Add cleanup for `loaded` on error paths or restructure with a cleanup label.

**File descriptor leak (line ~231):**
If `rte_cfgfile_save()` succeeds but `rte_cfgfile_close()` fails, or if loading fails, the temporary file is not removed (the `remove(filename)` is only reached at the end of the function).
Move file cleanup to an error path or use a cleanup label.

### Warnings

None beyond the errors above.

---

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

### Errors

**Resource leak on error path (line ~507, ~517):**
The `cfgfile` is loaded but not closed if `rte_cfgfile_has_entry()`, `rte_cfgfile_set_entry()`, `rte_cfgfile_add_entry()`, or `rte_cfgfile_save()` fail.
Each early return via `TEST_ASSERT*` leaks the cfgfile handle.

**Suggested fix:**
Add cleanup on all error paths, similar to the pattern in Patch 2.

**Resource leak on error path (line ~533):**
The `loaded` cfgfile is not closed if any of the assertions after loading fail (value checks, etc.).

**File descriptor leak:**
The temporary file is not removed if any assertion fails before the final `remove(filename)`.

### Warnings

None beyond the errors above.

---

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

### Errors

None. This patch is correct.

### Warnings

None.

---

## Summary

**Patches 2 and 3** have **resource leak errors** on multiple error paths where `rte_cfgfile` handles and temporary files are not cleaned up when assertions fail. The TEST_ASSERT macros cause early return, skipping the cleanup code at the end of the function.

**Recommended fix pattern** for all affected test functions:
Use a cleanup label and `goto` to ensure resources are always released, even on assertion failure. Or check each return value explicitly and clean up before calling TEST_ASSERT.

Example:
```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);
	if (cfgfile == NULL) {
		test_result = -1;
		goto cleanup;
	}

	ret = rte_cfgfile_add_section(cfgfile, "section1");
	if (ret != 0) {
		test_result = -1;
		goto cleanup;
	}
	/* ... similar for all operations ... */

	ret = make_tmp_file(filename, "create_save", "");
	if (ret != 0) {
		test_result = -1;
		goto cleanup;
	}

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

	rte_cfgfile_close(cfgfile);
	cfgfile = NULL;

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

	/* ... validation checks ... */
	value = rte_cfgfile_get_entry(loaded, "section1", "key1");
	if (strcmp("value1", value) != 0) {
		test_result = -1;
		goto cleanup;
	}
	/* ... more checks ... */

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

**Patch 1** and **Patch 4** are correct and have no issues.