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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

None identified.

## WARNINGS

### Missing test coverage for TSC timestamp path

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

The test `test_pcapng_timestamp()` only tests the user-supplied timestamp path (non-zero timestamp). There is no test that explicitly verifies that passing timestamp=0 to `rte_pcapng_copy()` still works correctly and produces a valid TSC-based timestamp in the output file.

**Suggested fix:** Add a test case that calls `rte_pcapng_copy()` with timestamp=0, writes the packet, reads it back with libpcap, and verifies the timestamp is reasonable (within a few seconds of the wall clock, similar to the `tsc_ns` check already present).

---

### Incomplete error handling documentation

**File:** `lib/pcapng/rte_pcapng.h`, line 139-140

The function documentation states that NULL is returned "on error such as invalid port, invalid timestamp or out of memory", but it does not specify what `rte_errno` values correspond to each error condition. The code sets `rte_errno = EINVAL` for the bit-63-set case, but this is not documented.

**Suggested fix:** Add a `@retval` or `@note` section documenting errno values:
```c
 * @return
 *   - The pointer to the new mbuf formatted for pcapng_write
 *   - NULL on error. rte_errno is set to:
 *     EINVAL if timestamp has bit 63 set,
 *     or the error from rte_pktmbuf_copy() on allocation failure.
```

---

### Asymmetric symbol export documentation

**File:** `lib/pcapng/rte_pcapng.c`, line 448

The new function `rte_pcapng_tsc_to_ns()` uses `RTE_EXPORT_EXPERIMENTAL_SYMBOL`, which is correct. However, the modified function `rte_pcapng_copy()` does not have any `RTE_EXPORT_*` annotation in the `.c` file. Since this is an existing stable function (not newly experimental), it should have `RTE_EXPORT_SYMBOL` to match the pattern for stable API.

The patch modifies the signature of `rte_pcapng_copy()` but does not add the export annotation. This is inconsistent with the guideline that stable functions should be annotated.

**Suggested fix:** Add `RTE_EXPORT_SYMBOL(rte_pcapng_copy)` before the function definition in `rte_pcapng.c`, similar to other stable functions in the codebase.

**Note:** The release notes correctly document this as an API change in 26.11 (ABI version 27), so versioning is not required per the commit message. However, the export annotation itself should still be present for consistency.

---

## INFO

### Pre-existing: pcapng_timestamp() not validated against wall clock

**File:** `lib/pcapng/rte_pcapng.c` (not touched by patch, observation only)

The test added in this patch (`test_pcapng_timestamp`) validates that `rte_pcapng_tsc_to_ns()` produces a result within 100ms of the wall clock. However, there is no test that the *existing* TSC-based path (when `rte_pcapng_copy()` is called with timestamp=0) also produces correct timestamps. The test `test_long_timestamp()` validates year-2262 handling but not that the converted TSC matches the wall clock in normal operation.

This is a pre-existing gap in test coverage, not introduced by this patch. Consider adding such a test in a follow-up to ensure `pcapng_timestamp()` calibration is working correctly.

---

Review-Result: WARNING