|WARNING| [v1] dts: fix TRex packet stream handling for line rate testing

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK DTS Patch Review

## Summary
This patch addresses TRex traffic generator issues for line rate testing by creating bidirectional packet streams and enabling promiscuous mode. The implementation has several **correctness bugs** and **style issues** that must be addressed.

---

## ERRORS (Must Fix)

### Correctness Bugs

**1. Missing error check on `packet[IP]` access**

In `_create_packet_stream()`, the code accesses `packet[IP].dst` without verifying the packet contains an IP layer:

```python
ip_prefix: str = direction_packet[IP].dst.rsplit(".", 1)[0]
```

If `packet` does not have an IP layer (e.g., pure L2 frame, IPv6, or non-IP protocol), this will raise a `KeyError` or `IndexError`, causing the test to fail with an unhelpful error message.

**Fix:** Add a check before accessing the IP layer:

```python
if IP not in direction_packet:
    raise ValueError("Packet must contain an IP layer for stream creation")
ip_prefix: str = direction_packet[IP].dst.rsplit(".", 1)[0]
```

**2. Hardcoded MAC address may not match topology**

The reverse packet destination MAC is hardcoded:

```python
reverse.dst = self._topology.sut_port_egress.mac_address
```

but the source MAC is hardcoded to `"52:00:00:00:00:00"`. This may not match the actual MAC address of the TRex port receiving return traffic, causing L2 forwarding issues.

**Fix:** Use the actual TRex port MAC addresses from the topology configuration or make this configurable.

---

**3. IP destination modification may corrupt packet checksums**

Modifying `direction_packet[IP].dst` in a loop without calling `del direction_packet[IP].chksum` will result in packets with incorrect IP checksums (Scapy caches checksums):

```python
for index in range(2):
    direction_packet[IP].dst = f"{ip_prefix}.{index}"
```

**Fix:** Delete the checksum field before modifying the packet, or rebuild the packet:

```python
for index in range(2):
    pkt = direction_packet.copy()
    pkt[IP].dst = f"{ip_prefix}.{index}"
    if IP in pkt:
        del pkt[IP].chksum
    self._shell.send_command(f"{packet_var}={pkt.command()}")
```

---

**4. Resource leak: `_stream_names_by_port` grows unbounded**

`_stream_names_by_port` is populated in `_create_packet_stream()` but never cleared. If `calculate_traffic_and_stats()` is called multiple times (e.g., in a loop of tests), streams accumulate in the TRex client, consuming memory and potentially causing conflicts.

**Fix:** Clear streams before creating new ones:

```python
def _create_packet_stream(self, packet: Packet) -> None:
    # Clear any existing streams
    if hasattr(self, '_stream_names_by_port'):
        self._shell.send_command(f"{self.stl_client_name}.remove_all_streams(ports=[0, 1])")
    self._stream_names_by_port = {}
    # ... rest of function
```

---

**5. Packet copy aliasing issue**

The code creates `forward` and `reverse` by calling `.copy()` on the original packet:

```python
forward: Packet = packet.copy()
reverse: Packet = packet.copy()
reverse.src = "52:00:00:00:00:00"
```

Scapy's `.copy()` is a shallow copy for some nested layers. If the packet contains mutable nested structures (e.g., payloads, options), modifications to `reverse` may affect `forward`. Use deep copy or rebuild:

```python
forward: Packet = packet.copy()
reverse: Packet = packet.__class__(bytes(packet))  # Deep copy via serialization
```

---

**6. Missing validation of `_topology` initialization**

`_setup_trex_client()` references `self._stream_names_by_port`, which is only initialized in `_create_packet_stream()`. If the methods are called out of order (or `_create_packet_stream()` fails before completion), this causes an `AttributeError`.

**Fix:** Initialize `_stream_names_by_port` in `setup()`:

```python
def setup(self, topology: Topology):
    super().setup(topology)
    self._topology = topology
    self._stream_names_by_port = {}
```

---

**7. Error handling path may leak partially created streams**

If `_shell.send_command()` fails partway through the loop in `_create_packet_stream()`, some streams are created but `_stream_names_by_port` is incomplete. Subsequent calls to `_setup_trex_client()` will not reference the orphaned streams, leaving them in TRex memory.

**Fix:** Wrap stream creation in error handling:

```python
try:
    for port_id, direction_packet in ((0, forward), (1, reverse)):
        # ... create streams
except Exception:
    self._shell.send_command(f"{self.stl_client_name}.remove_all_streams(ports=[0, 1])")
    self._stream_names_by_port = {}
    raise
```

---

### API and Process Compliance

**8. Missing experimental API marker on new public function signature change**

The `assess_performance_by_packet()` function in `api/packet.py` now calls `adjust_addresses([packet])[0]`, changing its behavior. This is an API-visible change (the packet's addresses are modified before being sent), but there is no indication that this function is experimental or that this change is documented in release notes.

**Fix:** If `assess_performance_by_packet()` is part of the DTS public API, this change requires a release note entry and potentially an experimental marker if it's a new function.

---

## WARNINGS (Should Fix)

### Code Quality

**9. Ambiguous error message on non-link-down failures**

The error message "TRex refused to start traffic" is raised when `error_text` does not contain "link is DOWN", but `error_text` may contain unrelated exceptions (Python tracebacks, TRex API errors, etc.) that are not distinguished:

```python
if "link is DOWN" not in error_text:
    raise SSHTimeoutError(f"TRex refused to start traffic: {error_text.strip()}")
```

**Suggestion:** Use a more specific exception type or parse the TRex error more carefully to avoid masking unrelated failures as SSH timeouts.

---

**10. Hardcoded port list `[0, 1]` reduces code reusability**

The port list `[0, 1]` is hardcoded throughout (`reset(ports=[0, 1])`, `start(ports=[0, 1])`). If the topology uses different port IDs or a different number of ports, this code will fail.

**Suggestion:** Derive port IDs from `self._topology` or make them configurable:

```python
port_ids = [self._topology.tg_port_ingress.id, self._topology.tg_port_egress.id]
self._shell.send_command(f"{self.stl_client_name}.reset(ports={port_ids})")
```

---

**11. `.mailmap` entry lacks affiliation or email context**

The new `.mailmap` entry:

```
+Dillon Ladd <dladd at iol.unh.edu>
```

is a standalone entry with no mapping. This is acceptable, but typically `.mailmap` entries either canonicalize multiple emails to one (`Old Email <old at example.com> New Email <new at example.com>`) or fix name spellings. A standalone entry suggests this is the contributor's first patch, which is fine but unusual.

**Suggestion:** No action required unless there are known alternate spellings or emails for this contributor.

---

### Style Issues

**12. Inconsistent string quote style**

The code mixes single and double quotes:

```python
reverse.src = "52:00:00:00:00:00"
reverse.dst = self._topology.sut_port_egress.mac_address
```

vs.

```python
mult = f"'{send_mpps}mpps'" if send_mpps else "'100%'"
```

While Python allows both, DPDK typically prefers double quotes for strings. This is minor but reduces consistency.

---

**13. Magic numbers without named constants**

The loop `range(2)` (two streams per port) and `percentage=50` (50% rate per stream) are hardcoded. If the number of streams changes, these must be updated in lockstep.

**Suggestion:**

```python
STREAMS_PER_PORT = 2
STREAM_PERCENTAGE = 100 // STREAMS_PER_PORT

for index in range(STREAMS_PER_PORT):
    # ...
    mode=trex.stl.trex_stl_streams.{self._streaming_mode}(percentage={STREAM_PERCENTAGE}),
```

---

**14. Missing type hints on `_stream_names_by_port` initialization**

The field `_stream_names_by_port` is declared with a type hint in the class body:

```python
_stream_names_by_port: dict[int, list[str]]
```

but is assigned in `_create_packet_stream()` without re-declaring the type. This is acceptable, but initializing it in `setup()` (per Error #6 fix) would be clearer.

---

## INFO (Consider)

**15. Potential performance impact of `packet.command()` serialization**

Calling `packet.command()` in a loop (once per stream) serializes the packet to a Scapy command string each time. If packets are large or the topology has many ports, this could be slow. Consider caching the serialized packet command if it does not vary.

---

**16. TRex API version assumptions**

The code assumes specific TRex API behavior (`_start_rc.good()`, `_start_rc.err`, attribute existence). If the TRex Python API changes between versions, this could break silently. Document the required TRex version in comments or configuration.

---

**17. Logging improvement opportunity**

The retry loop logs `f"Traffic start refused (attempt {attempt + 1} of {retry_attempts})"` but does not log when traffic successfully starts. Adding a success log would aid debugging:

```python
if started:
    self._logger.info("Traffic started successfully")
```

---

## Final Checklist

- [x] Correctness: **7 errors** (missing IP check, hardcoded MAC, checksum corruption, resource leak, copy aliasing, missing init, stream leak on error)
- [x] API: **1 error** (undocumented behavior change in `assess_performance_by_packet`)
- [x] Style: **3 warnings** (error message clarity, hardcoded ports, `.mailmap` entry)
- [x] Code quality: **3 warnings** (quote style, magic numbers, type hints)
- [x] Process: Commit message and SPDX not reviewed per instructions

---

**Recommendation:** Address all 8 errors before merging. The warnings are recommended improvements but not blockers.