|WARNING| [v8] pcapng: add user-supplied timestamp support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-20

# DPDK Patch Review

## ERRORS

**None**

## WARNINGS

**1. Missing release notes for API change**

Line: lib/pcapng/rte_pcapng.h:+148
```c
		enum rte_pcapng_direction direction,
		const char *comment, uint64_t timestamp);
```

The patch changes `rte_pcapng_copy()` by adding a `timestamp` parameter. While the API Changes section in `release_26_11.rst` is updated (lines +548-552), there is no corresponding entry in the New Features section. Significant API changes should be documented in both sections: API Changes notes the breaking change, and New Features explains the capability it enables.

---

**2. Test does not verify TSC path on failure to reject invalid timestamp**

Line: app/test/test_pcapng.c:+754-760
```c
	mc = rte_pcapng_copy(port_id, 0, &mbfs.mb[0], mp,
			     rte_pktmbuf_pkt_len(&mbfs.mb[0]),
			     RTE_PCAPNG_DIRECTION_IN, NULL, UINT64_C(1) << 63);
	if (mc != NULL || rte_errno != EINVAL) {
		printf("rte_pcapng_copy accepted a timestamp with bit 63 set\n");
		rte_pktmbuf_free(mc);
		goto fail;
	}
```

The test verifies that a caller-supplied timestamp with bit 63 set is rejected, but does not verify that the TSC path (timestamp=0) still works correctly after this change. The new test should include a case with `timestamp=0` to ensure the default TSC behavior is preserved.

---

**3. Potential confusion in doc comment wording**

Line: lib/pcapng/rte_pcapng.h:+131-136
```c
 * @param timestamp
 *   Time of the packet in nanoseconds since the Unix epoch,
 *   for example a hardware timestamp. Bit 63 must be zero.
 *   If zero, the current TSC is used,
 *   and rte_pcapng_write_packets() converts it to the epoch.
```

The phrase "Bit 63 must be zero" followed by "If zero" could be read as requiring the entire timestamp to be zero to use TSC. Consider rephrasing for clarity:
```c
 * @param timestamp
 *   Time of the packet in nanoseconds since the Unix epoch,
 *   for example a hardware timestamp. Bit 63 must not be set.
 *   If the timestamp is 0, the current TSC is used instead,
 *   and rte_pcapng_write_packets() converts it to the epoch.
```

This makes it clear that "zero" refers to the full value, not just bit 63, and that the TSC is a fallback when the caller has no timestamp.

---

## INFO

**1. Consider adding a test case for TSC=0 behavior**

While the test verifies that a custom timestamp is correctly stored, it does not verify that `timestamp=0` (the default/TSC path) still produces valid timestamps in the output file. This would strengthen confidence that the change does not break the existing behavior.

**2. Test cleanup could use a helper**

Lines: app/test/test_pcapng.c:+807-809
```c
fail:
	if (pcapng)
		rte_pcapng_close(pcapng);
	remove(file_name);
	return -1;
```

This pattern (close if non-null, remove temp file, return -1) appears in multiple tests. A cleanup helper function would reduce duplication, but this is a minor style preference and not a requirement for this patch.

Review-Result: WARNING