|WARNING| [v3,1/1] pcapng: add user-supplied timestamp support

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch adds user-supplied timestamp support to the pcapng library. The implementation uses a sentinel bit (bit 63) to distinguish between TSC-based timestamps and caller-supplied epoch-ns timestamps.

---

## Errors

### 1. Exported symbol name does not match function signature
The `RTE_EXPORT_SYMBOL` macro references `rte_pcapng_copy` but is placed on `rte_pcapng_copy_ts()`.

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

```c
/* Current code - WRONG */
RTE_EXPORT_SYMBOL(rte_pcapng_copy)
struct rte_mbuf *
rte_pcapng_copy_ts(uint16_t port_id, uint32_t queue,
```

**Fix:**
```c
RTE_EXPORT_SYMBOL(rte_pcapng_copy_ts)
struct rte_mbuf *
rte_pcapng_copy_ts(uint16_t port_id, uint32_t queue,
```

The exported symbol name must match the function being exported. This mismatch will cause linker errors or incorrect symbol tables.

---

### 2. Missing experimental API tag for new function
The new `rte_pcapng_copy_ts()` function is a public API addition but is not marked `__rte_experimental`.

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

**Fix:**
```c
__rte_experimental
struct rte_mbuf *
rte_pcapng_copy_ts(uint16_t port_id, uint32_t queue,
		const struct rte_mbuf *m, struct rte_mempool *mp,
		uint32_t length,
		enum rte_pcapng_direction direction, const char *comment, uint64_t ts);
```

New API functions must be marked as experimental per DPDK policy.

---

### 3. Missing RTE_EXPORT_EXPERIMENTAL_SYMBOL macro for new API
The new `rte_pcapng_copy_ts()` function needs `RTE_EXPORT_EXPERIMENTAL_SYMBOL` instead of `RTE_EXPORT_SYMBOL` since it is a new experimental API.

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

**Fix:**
```c
RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pcapng_copy_ts, 26.11)
struct rte_mbuf *
rte_pcapng_copy_ts(uint16_t port_id, uint32_t queue,
```

New experimental API must use `RTE_EXPORT_EXPERIMENTAL_SYMBOL` with the release version (assuming 26.11 based on the 2026 date; adjust if different).

---

## Warnings

### 1. Missing release notes update
This patch adds new public API (`rte_pcapng_copy_ts()`) but does not include changes to the release notes documenting the addition.

**Expected:** Update `doc/guides/rel_notes/release_*.rst` (current release) to document:
- New `rte_pcapng_copy_ts()` function
- Purpose: allow applications to supply hardware PTP timestamps
- Behavior change: `rte_pcapng_copy()` preserved as inline wrapper

---

### 2. Documentation: function comment could be clearer about zero behavior
The Doxygen comment for the `ts` parameter states "If zero, the current TSC is captured" but does not clarify that the conversion happens at write time, not at copy time.

**Location:** `lib/pcapng/rte_pcapng.h:133-135`

**Suggestion:**
```c
 * @param ts
 *   Packet timestamp in nanoseconds since the Unix epoch. If zero,
 *   the current TSC is captured and will be converted to epoch
 *   nanoseconds by rte_pcapng_write_packets() when the packet is written.
```

This makes the deferred conversion explicit in the API contract.

---

### 3. Potential portability concern with sentinel bit assumption
The implementation assumes TSC values will never reach bit 63 and epoch-ns timestamps will not exceed bit 63 until 2554. While the commit message documents this, it may be worth adding a compile-time or runtime assertion to detect violations early.

**Suggestion (optional):**
Add a static assertion or comment in `rte_pcapng_copy_ts()` checking that the TSC frequency is within expected bounds, or add a runtime check in debug builds that validates the TSC value does not have bit 63 set before storing.

This is borderline Info-level but flagged as Warning because silent corruption in the year 2554 or on hypothetical high-frequency TSC systems would be hard to debug.

---

## Info

### 1. Inline function placement
The new `rte_pcapng_copy()` inline wrapper is correctly placed in the header after the `rte_pcapng_copy_ts()` declaration. No issue, but worth noting this follows best practice for inline wrappers preserving backward compatibility.

---

## Additional Notes

### What was NOT flagged (per guidelines):
- Copyright years in `.mailmap` (not subject to AI review)
- Commit message format (checked by checkpatch)
- Symbol map changes (auto-generated from `RTE_EXPORT_*` macros)

### Correctness verification:
- No use-after-free, resource leaks, or race conditions detected
- Error paths correctly propagate failures from `pcapng_vlan_insert()` and `rte_pktmbuf_copy()`
- Timestamp handling logic is sound given the documented assumptions
- No buffer overflows, out-of-bounds access, or uninitialized variable use

The core implementation is solid; the issues are primarily process compliance (experimental tag, export symbol naming, release notes).