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

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: pcapng: add user-supplied timestamp support

## Summary

This patch adds support for caller-supplied timestamps in the pcapng library by introducing `rte_pcapng_copy_ts()` and `rte_pcapng_tsc_to_ns()`. The implementation uses bit 63 of the timestamp as a sentinel to distinguish between TSC values (to be converted at write time) and epoch-ns values (already converted).

---

## Errors

### 1. Missing `RTE_EXPORT_SYMBOL` for `rte_pcapng_tsc_to_ns`

**Location:** `lib/pcapng/rte_pcapng.c:744`

The function `rte_pcapng_tsc_to_ns()` is declared with `RTE_EXPORT_EXPERIMENTAL_SYMBOL` but the exported function body is missing the corresponding export macro.

**Current code:**
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pcapng_tsc_to_ns, 26.07)
uint64_t
rte_pcapng_tsc_to_ns(const rte_pcapng_t *self, uint64_t tsc)
{
	return tsc_to_ns_epoch(&self->clock, tsc);
}
```

**Fix:** The macro placement appears correct. However, verify that `tsc_to_ns_epoch()` is accessible and that `self->clock` is properly initialized when this function is called. The code assumes `self` is non-NULL but does not check it.

**Add NULL check:**
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pcapng_tsc_to_ns, 26.07)
uint64_t
rte_pcapng_tsc_to_ns(const rte_pcapng_t *self, uint64_t tsc)
{
	if (self == NULL)
		return 0;
	return tsc_to_ns_epoch(&self->clock, tsc);
}
```

---

## Warnings

### 1. Missing release notes entry

**Location:** Entire patch

The patch introduces new public API functions (`rte_pcapng_copy_ts()` and `rte_pcapng_tsc_to_ns()`) but does not include corresponding updates to the release notes. According to the guidelines, "Changes to API require release notes" and "New API functions must have release notes."

**Fix:** Add an entry to `doc/guides/rel_notes/release_26_07.rst` documenting the new experimental API.

### 2. Missing functional tests

**Location:** Entire patch

The guidelines state "New API functions must have hooks in `app/testpmd` and tests in the functional test suite." This patch adds two new API functions but does not include corresponding test coverage in `app/test/`.

**Fix:** Add unit tests to `app/test/` that:
- Verify `rte_pcapng_copy_ts()` with explicit timestamp (ts != 0)
- Verify `rte_pcapng_copy_ts()` with ts=0 (TSC capture)
- Verify `rte_pcapng_tsc_to_ns()` conversion correctness
- Verify the sentinel bit mechanism

### 3. Doxygen incomplete for `rte_pcapng_tsc_to_ns`

**Location:** `lib/pcapng/rte_pcapng.h:217`

The Doxygen comment for `rte_pcapng_tsc_to_ns()` does not document behavior when `self` is NULL.

**Fix:**
```c
/**
 * Convert a TSC value to nanoseconds since the Unix epoch.
 *
 * Uses the same calibrated clock reference as the capture file so that
 * the result is consistent with timestamps written by
 * rte_pcapng_write_packets(). The conversion is drift-compensated and
 * uses a pre-computed reciprocal multiplier (no integer division).
 *
 * Typical use: convert a TSC timestamp captured close to packet arrival
 * (e.g., from a PMD or hardware register) to an epoch-ns value before
 * passing it to rte_pcapng_copy_ts().
 *
 * @param self
 *   The handle to the packet capture file. Must not be NULL.
 * @param tsc
 *   TSC value to convert.
 * @return
 *   Nanoseconds since the Unix epoch corresponding to @p tsc,
 *   or 0 if @p self is NULL.
 */
```

### 4. Timestamp overflow not documented

**Location:** `lib/pcapng/rte_pcapng.h:95-113`

The Doxygen for `rte_pcapng_copy_ts()` states that bit 63 is used as a sentinel but does not warn callers about the valid timestamp range. While the implementation comment says "epoch-ns values stay below bit 63 until 2554," this constraint is not documented in the API contract.

**Fix:** Add to `@param ts` documentation:
```c
 * @param ts
 *   Packet timestamp in nanoseconds since the Unix epoch. If zero, the
 *   current TSC is captured and converted to epoch ns by
 *   rte_pcapng_write_packets() when the packet is written.
 *   Must not have bit 63 set (timestamps below 2^63 ns = year 2554).
```

---

## Info

### 1. Const correctness on `rte_pcapng_tsc_to_ns` parameter

**Location:** `lib/pcapng/rte_pcapng.h:217`

The function signature uses `const rte_pcapng_t *self`, which is correct for a read-only operation. This is good practice.

### 2. Code comment quality

**Location:** `lib/pcapng/rte_pcapng.c:692-701, 781-792`

The inline comments explaining the timestamp handling mechanism are clear and helpful. The explanation of the sentinel bit is particularly well done.

---

## Additional Observations

1. **Correctness of sentinel bit mechanism:** The logic appears sound. TSC counters increment much slower than the sentinel threshold, and epoch-ns timestamps won't reach bit 63 until far in the future (2554). The masking at write time (`timestamp &= ~(UINT64_C(1) << 63)`) correctly clears the sentinel before conversion.

2. **ABI compatibility:** The patch correctly preserves `rte_pcapng_copy()` as a real exported symbol by making it a wrapper around `rte_pcapng_copy_ts(..., 0)`. This maintains backward compatibility.

3. **Zero timestamp semantics:** Using `ts == 0` to mean "capture TSC now" is a reasonable choice since Unix epoch time zero (1970) is not a meaningful packet capture time in practice.

---

## Summary of Required Changes

**Must fix (Errors):**
- Add NULL pointer check in `rte_pcapng_tsc_to_ns()`

**Should fix (Warnings):**
- Add release notes entry for new experimental API
- Add functional tests for new API functions
- Complete Doxygen documentation for NULL handling and timestamp constraints