|WARNING| dev: fix hotplug notification to secondary

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary

This patch fixes a critical logic bug in multi-process hotplug notification and adds comprehensive unit tests. The core fix is correct, but there are several style issues and one potential correctness concern in the test code.

---

## Errors

### 1. Potential use-after-free in `run_primary_hotplug_test()`

**File:** `app/test/test_dev_hotplug.c`  
**Lines:** 290-291

The code calls `rte_memzone_free(mz)` at line 290, then attempts to access `dev` (which may reference memory in that memzone or related structures) immediately after. While `dev` was obtained before the free, if the device removal at line 284 failed or was skipped, the pointer could be stale.

```c
out:
	if (pid > 0) {
		int wstatus;

		waitpid(pid, &wstatus, 0);
		if (WIFEXITED(wstatus))
			printf("Primary: secondary exited with status %d\n", WEXITSTATUS(wstatus));
	}
	rte_memzone_free(mz);  /* <-- memzone freed here */

	if (test_result == TEST_SUCCESS)
		printf("== MP hotplug test passed ==\n");
	else
		printf("== MP hotplug test FAILED ==\n");

	return test_result;
```

**Fix:** Move the device cleanup check and `rte_dev_remove()` call to before the `out:` label, or ensure it completes before `rte_memzone_free()`.

```c
out:
	if (pid > 0) {
		int wstatus;
		waitpid(pid, &wstatus, 0);
		if (WIFEXITED(wstatus))
			printf("Primary: secondary exited with status %d\n", WEXITSTATUS(wstatus));
	}

	/* Clean up device before freeing memzone */
	dev = find_device_by_name(HOTPLUG_TEST_DEV);
	if (dev != NULL) {
		printf("Primary: removing device %s\n", HOTPLUG_TEST_DEV);
		rte_dev_remove(dev);
	}

	if (mz != NULL)
		rte_memzone_free(mz);

	/* ... print result ... */
```

---

## Warnings

### 1. Missing release notes update

**File:** Release notes (not modified)

This patch fixes Bugzilla #1963 and adds new public test APIs (`REGISTER_FAST_TEST` registration). While test-only changes typically do not require release notes, the bug fix in `eal_common_dev.c` is a correctness fix for multi-process hotplug that could affect users. The release notes should mention the fix.

**Suggested addition to current release notes (`doc/guides/rel_notes/release_26_07.rst` or similar):**

```rst
* **Fixed multi-process hotplug notification.**

  Fixed a logic error in ``rte_dev_probe()`` and ``rte_dev_remove()``
  that prevented secondary processes from receiving device hotplug
  notifications in multi-process configurations.
```

### 2. `REGISTER_FAST_TEST` uses `NOHUGE_SKIP` / `ASAN_SKIP` instead of `ASAN_FAILS`

**File:** `app/test/test_dev_hotplug.c`  
**Line:** 320

```c
REGISTER_FAST_TEST(dev_hotplug_mp_autotest, NOHUGE_SKIP, ASAN_SKIP, test_dev_hotplug_mp);
```

The test is skipped under ASAN, but the correct macro for tests that are incompatible with ASan should be `ASAN_FAILS` (based on the guidelines' mention of `ASAN_OK` / `ASAN_FAILS` as the two valid options). Check if `ASAN_SKIP` is a valid macro; if not, use `ASAN_FAILS`.

**Suggested fix (if `ASAN_SKIP` is not defined):**

```c
REGISTER_FAST_TEST(dev_hotplug_mp_autotest, NOHUGE_SKIP, ASAN_FAILS, test_dev_hotplug_mp);
```

### 3. Child process error path does not call `_exit()`

**File:** `app/test/test_dev_hotplug.c`  
**Lines:** 253

After `fork()`, the child process uses `exit(TEST_FAILED)` if `execv()` fails. In test code that may have registered cleanup handlers or opened files, `_exit()` is safer than `exit()` to avoid double-cleanup issues.

```c
if (setenv(RECURSIVE_ENV_VAR, "run_hotplug_secondary", 1) == 0)
	execv(argv[0], argv);
exit(TEST_FAILED);  /* Should be _exit() */
```

**Fix:**

```c
if (setenv(RECURSIVE_ENV_VAR, "run_hotplug_secondary", 1) == 0)
	execv(argv[0], argv);
_exit(TEST_FAILED);
```

### 4. Allocated strings in child process not freed before exec failure

**File:** `app/test/test_dev_hotplug.c`  
**Lines:** 240-252

The child process allocates 5 strings with `strdup()` but does not free them if `execv()` fails. While the process exits immediately, this is a minor resource leak on the error path.

**Fix (low priority):**

```c
argv[0] = strdup(prgname);
argv[1] = strdup("-l");
argv[2] = strdup(core_str);
argv[3] = strdup("--proc-type=secondary");
argv[4] = strdup(prefix);
argv[5] = NULL;

if (setenv(RECURSIVE_ENV_VAR, "run_hotplug_secondary", 1) != 0) {
	for (int i = 0; i < 5; i++)
		free(argv[i]);
	_exit(TEST_FAILED);
}
if (execv(argv[0], argv) != 0) {
	for (int i = 0; i < 5; i++)
		free(argv[i]);
	_exit(TEST_FAILED);
}
```

---

## Style Issues (Info)

### 1. Implicit comparison in boolean assignment

**File:** `lib/eal/common/eal_common_dev.c`  
**Lines:** 272, 431

```c
bool do_mp = internal_conf->no_shconf == 0;
```

While this is explicit comparison (which is correct per guidelines), the double-negative naming (`no_shconf == 0` to get "do multiprocess") is confusing. Consider renaming the local variable or adding a comment.

**Suggested improvement (comment):**

```c
/* Multi-process is enabled when shared config is NOT disabled */
bool do_mp = internal_conf->no_shconf == 0;
```

### 2. Missing NULL check on `strdup()` return values

**File:** `app/test/test_dev_hotplug.c`  
**Lines:** 240-245

`strdup()` can return NULL on allocation failure. The code does not check these before passing to `execv()`.

**Suggested fix:**

```c
argv[0] = strdup(prgname);
argv[1] = strdup("-l");
argv[2] = strdup(core_str);
argv[3] = strdup("--proc-type=secondary");
argv[4] = strdup(prefix);
if (!argv[0] || !argv[1] || !argv[2] || !argv[3] || !argv[4])
	_exit(TEST_FAILED);
argv[5] = NULL;
```

### 3. Inconsistent naming: `test_dev_hotplug_api` vs `test_dev_hotplug_mp`

**File:** `app/test/test_dev_hotplug.c`

The naming is consistent within the file but could be more descriptive. Consider `test_hotplug_single_process` and `test_hotplug_multiprocess` for clarity.

---

## Positive Observations

1. **Core bug fix is correct:** The change from `!!internal_conf->no_shconf` to `internal_conf->no_shconf == 0` correctly inverts the logic to enable multi-process notification when shared config is enabled.

2. **Test coverage is comprehensive:** The patch adds both single-process API tests and multi-process notification tests, which will catch regressions in this area.

3. **Atomic operations used correctly:** The test coordination uses `rte_atomic_*_explicit()` with appropriate memory ordering (`release` for signaling, `acquire` for waiting, `relaxed` for result storage).

4. **Resource cleanup attempted:** The test tries to remove the probed device in both success and failure paths.

---

## Recommendation

**Merge after addressing the use-after-free concern (Error #1) and adding release notes (Warning #1).**

The other warnings and style issues are minor and can be addressed in a follow-up patch if needed.