Conversation
No need to use rte_malloc or rte_memcpy in the short code to get MAC address. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
When used as single interface useful to support pass through of MTU setting to enable larger frames. Cleanup the transmit logic so that if packet sent exceeds MTU is an error. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
Save some space by using bool for flag values. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
The driver already handles multi-segment mbufs but did not report that in offload flags. Driver can easily insert vlan tag making testing easier. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
Consistently support nanosecond timestamps across all the variations of pcap PMD receive. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
Localize variables where possible. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Acked-by: Marat Khalili <marat.khalili@huawei.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Using volatile for statistics is not necessary since only one thread is allowed to operate on a queue at a time. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
Use rte_ether_addr structure to avoid memcpy and void *. When using pcap on a single interface, it is possible for driver to proxy the mac address set operation. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
This test was generated by Claude AI with some prompting and pointing at existing ring PMD test. It tests basic operations, timestamps, jumbo frame, and multiple queues. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: 0-day Robot <robot@bytheb.org>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe pull request introduces comprehensive PCAP PMD unit tests, adds timestamp offloading support with nanosecond precision, implements device operations (MTU set, MAC address set, clock read), and extends OS-dependent layers with interface configuration capabilities across Linux, FreeBSD, and Windows platforms. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/net/pcap/pcap_ethdev.c (1)
478-524: Missing mbuf cleanup ineth_pcap_tx.Unlike
eth_pcap_tx_dumper(line 436) andeth_tx_drop(line 467) which free all mbufs withrte_pktmbuf_free_bulk, this function does not free the transmitted mbufs. In DPDK, TX functions are responsible for freeing mbufs they consume. This will cause memory leaks.Proposed fix
tx_queue->tx_stat.pkts += num_tx; tx_queue->tx_stat.bytes += tx_bytes; tx_queue->tx_stat.err_pkts += nb_pkts - num_tx; + rte_pktmbuf_free_bulk(bufs, nb_pkts); + return nb_pkts; }
🤖 Fix all issues with AI agents
In @app/test/test_pmd_pcap.c:
- Around line 1367-1396: The test uses NS_PER_S but never includes
<rte_cycles.h>, so add a header include for rte_cycles.h near the other includes
to bring in the NS_PER_S definition; also replace the silent printf warning that
reports timestamp mismatch (the message printed when ts != expected) with a
failing assertion using TEST_ASSERT (or augment the existing check to call
TEST_ASSERT(ts == expected, "... expected=%"PRIu64" actual=%"PRIu64"", expected,
ts)) so timestamp mismatches fail the test rather than only logging a warning.
In @drivers/net/pcap/pcap_osdep_freebsd.c:
- Around line 84-105: The redundant socket error check in osdep_iface_mtu_set
should be removed: delete the second "if (s < 0) return -EINVAL;" (the earlier
socket() failure already returns), leaving the socket variable use intact;
ensure struct ifreq ifr remains zero-initialized (struct ifreq ifr = { 0 };),
copy the interface name with strlcpy(ifr.ifr_name, ifname, IFNAMSIZ) and set
ifr.ifr_mtu = mtu before calling ioctl and closing the socket.
In @drivers/net/pcap/pcap_osdep_windows.c:
- Around line 119-123: Add the missing osdep_iface_mac_set function to match the
declaration in pcap_osdep.h: implement a Windows stub named osdep_iface_mac_set
with the same signature as declared (use the same parameter names/unused
annotations as other Windows stubs) and return -ENOTSUP, mirroring
osdep_iface_mtu_set's behavior so the symbol is defined and the linker error is
resolved.
🧹 Nitpick comments (1)
drivers/net/pcap/pcap_ethdev.c (1)
330-341: Consider adding compile-time assertions to document struct compatibility.The cast from
struct timeval*tostruct timespec*is not unsafe—it relies on well-documented libpcap behavior: whenPCAP_TSTAMP_PRECISION_NANOis set, thetv_usecfield ofstruct timevalcontains nanoseconds instead of microseconds. This is standardized libpcap behavior (documented inpcap_get/set_tstamp_precision), and the code already acknowledges this in its comments. The struct layouts are compatible since both contain two time-related fields with identical offsets and sizes.However, consider adding
_Static_assertor similar compile-time assertions to explicitly validate thatoffsetof(struct timeval, tv_sec) == offsetof(struct timespec, tv_sec)and their sizes match. This would make the assumption explicit to future maintainers and catch any platform-specific surprises at compile time rather than runtime.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/test/meson.buildapp/test/test_pmd_pcap.cdrivers/net/pcap/pcap_ethdev.cdrivers/net/pcap/pcap_osdep.hdrivers/net/pcap/pcap_osdep_freebsd.cdrivers/net/pcap/pcap_osdep_linux.cdrivers/net/pcap/pcap_osdep_windows.c
🧰 Additional context used
🧬 Code graph analysis (5)
drivers/net/pcap/pcap_osdep.h (3)
drivers/net/pcap/pcap_osdep_freebsd.c (2)
osdep_iface_mac_set(61-82)osdep_iface_mtu_set(84-105)drivers/net/pcap/pcap_osdep_linux.c (2)
osdep_iface_mac_set(49-70)osdep_iface_mtu_set(72-91)drivers/net/pcap/pcap_osdep_windows.c (1)
osdep_iface_mtu_set(120-123)
drivers/net/pcap/pcap_osdep_freebsd.c (2)
drivers/net/pcap/pcap_osdep_linux.c (2)
osdep_iface_mac_set(49-70)osdep_iface_mtu_set(72-91)drivers/net/pcap/pcap_osdep_windows.c (1)
osdep_iface_mtu_set(120-123)
drivers/net/pcap/pcap_osdep_linux.c (2)
drivers/net/pcap/pcap_osdep_freebsd.c (2)
osdep_iface_mac_set(61-82)osdep_iface_mtu_set(84-105)drivers/net/pcap/pcap_osdep_windows.c (1)
osdep_iface_mtu_set(120-123)
app/test/test_pmd_pcap.c (3)
lib/net/rte_ip4.h (1)
rte_ipv4_cksum(156-162)lib/mbuf/rte_mbuf.h (4)
rte_pktmbuf_alloc_bulk(1068-1082)rte_pktmbuf_alloc(1046-1052)rte_pktmbuf_tailroom(1629-1634)rte_pktmbuf_append(1720-1735)lib/ethdev/rte_ethdev.h (1)
rte_eth_rx_burst(6347-6406)
drivers/net/pcap/pcap_osdep_windows.c (2)
drivers/net/pcap/pcap_osdep_freebsd.c (1)
osdep_iface_mtu_set(84-105)drivers/net/pcap/pcap_osdep_linux.c (1)
osdep_iface_mtu_set(72-91)
🪛 Cppcheck (2.19.0)
app/test/test_pmd_pcap.c
[error] 1295-1295: There is an unknown macro here somewhere. Configuration is required. If RTE_ETHER_ADDR_PRT_FMT is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (17)
drivers/net/pcap/pcap_osdep.h (1)
17-18: LGTM!The new function declarations are consistent with the implementations in the OS-specific source files and follow the existing patterns in this header.
app/test/meson.build (1)
144-144: LGTM!The test file dependencies are correctly configured. The internal dependencies (
net_pcap,ethdev,bus_vdev,net) and external dependency (pcap) match what the test file requires.Also applies to: 220-220
drivers/net/pcap/pcap_osdep_linux.c (2)
49-70: LGTM!The
osdep_iface_mac_setimplementation correctly uses the Linux-specificSIOCSIFHWADDRioctl with proper error handling and resource cleanup.
72-91: LGTM!The
osdep_iface_mtu_setimplementation is correct. Minor note: usesPF_INETwhileosdep_iface_mac_setusesAF_INET- both are equivalent but could be made consistent if desired.drivers/net/pcap/pcap_osdep_freebsd.c (1)
61-82: LGTM!The
osdep_iface_mac_setimplementation correctly uses the FreeBSD-specificSIOCSIFLLADDRioctl withAF_LINKaddress family.app/test/test_pmd_pcap.c (5)
1-25: LGTM!Good test file structure with appropriate includes for DPDK APIs, libpcap, and standard C libraries. The copyright and license header are correct.
90-123: LGTM!The timestamp helper functions correctly use the DPDK dynamic field API for accessing mbuf timestamps. The
timestamp_init()function properly handles the case where the dynamic field isn't registered.
548-583: LGTM!The
alloc_jumbo_mbuffunction correctly handles multi-segment mbuf allocation with proper cleanup on failure. The pattern of trackingprevpointer for chaining segments is clean.
1478-1581: LGTM!The
test_multi_tx_queuetest is well-structured with proper resource allocation, transmission to multiple queues, and thorough validation of packet counts in each output file.
1769-1813: LGTM!Test setup creates the required mempool, and teardown properly cleans up all temporary files and frees the mempool. The null-check pattern before
unlink()prevents issues with uninitialized paths.drivers/net/pcap/pcap_ethdev.c (7)
8-23: LGTM!The new includes (
stdbool.h,rte_time.h,rte_reciprocal.h) are appropriate for the bool type usage and timestamp calculations with reciprocal division.
366-384: LGTM!The
calculate_timestampfunction correctly uses reciprocal division for efficient conversion of cycles to time, avoiding expensive division operations in the data path.
529-583: LGTM!The reworked
open_iface_livefunction properly uses the pcap creation/activation sequence to set nanosecond timestamp precision, immediate mode, promiscuous mode, and snapshot length. Error handling and cleanup are correct.
793-801: LGTM!The
eth_dev_configurefunction correctly propagates the timestamp offload setting from the device configuration to internal state.
1078-1110: LGTM!The new device operations are well-implemented:
eth_mtu_setcorrectly delegates to OS-dependent layer only for single-interface modeeth_dev_macaddr_setappropriately returns-ENOTSUPfor non-single-interface modeeth_rx_clockreturns UTC-based timestamps consistent with pcap file timestamps
1284-1300: LGTM!The refactored
process_bool_flaghelper is cleaner than having separate handlers forphy_macandinfinite_rx. It correctly handles empty values, "0", and "1" with appropriate error reporting.
1531-1538: LGTM!The timestamp initialization on first probe is correct. Recording
start_timeandstart_cyclestogether ensures consistent timestamp generation, andhz_invis properly initialized for reciprocal division.
| for (i = 0; i < received; i++) { | ||
| struct rte_mbuf *m = mbufs[i]; | ||
|
|
||
| TEST_ASSERT(mbuf_has_timestamp(m), | ||
| "Packet %u missing timestamp flag", i); | ||
|
|
||
| /* PCAP PMD stores timestamp in nanoseconds */ | ||
| rte_mbuf_timestamp_t ts = mbuf_timestamp_get(mbufs[i]); | ||
| uint64_t expected = (uint64_t)base_sec * NS_PER_S | ||
| + (uint64_t)i * usec_increment * 1000; | ||
|
|
||
| if (ts != expected) | ||
| printf("Packet %u: timestamp mismatch, expected=%"PRIu64" actual=%"PRIu64"\n", | ||
| i, expected, ts); | ||
|
|
||
| /* Verify monotonically increasing timestamps */ | ||
| if (i > 0) { | ||
| TEST_ASSERT(ts >= prev_ts, | ||
| "Packet %u: timestamp not monotonic %"PRIu64" > %"PRIu64, | ||
| i, prev_ts, ts); | ||
| } | ||
| prev_ts = ts; | ||
| } | ||
|
|
||
| rte_pktmbuf_free_bulk(mbufs, received); | ||
| cleanup_pcap_vdev("net_pcap_ts", port_id); | ||
|
|
||
| printf("RX timestamp PASSED: %u packets with valid timestamps\n", received); | ||
| return TEST_SUCCESS; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Check where NS_PER_S is defined
echo "=== Searching for NS_PER_S definition ==="
rg -n "define NS_PER_S|NS_PER_S\s*=" --type h --type cRepository: ovsrobot/dpdk
Length of output: 429
🏁 Script executed:
#!/bin/bash
# Step 2: Examine the actual test file to see includes and NS_PER_S usage
echo "=== Checking app/test/test_pmd_pcap.c header includes ==="
head -50 app/test/test_pmd_pcap.c | grep -E "^#include"Repository: ovsrobot/dpdk
Length of output: 520
🏁 Script executed:
#!/bin/bash
# Step 3: Look at the specific test function to confirm the code structure
echo "=== Examining lines around 1367-1396 in test_pmd_pcap.c ==="
sed -n '1360,1400p' app/test/test_pmd_pcap.cRepository: ovsrobot/dpdk
Length of output: 1378
🏁 Script executed:
#!/bin/bash
# Check if NS_PER_S is actually used and available in test_pmd_pcap.c
echo "=== Checking NS_PER_S usage in test_pmd_pcap.c ==="
grep -n "NS_PER_S" app/test/test_pmd_pcap.c | head -20Repository: ovsrobot/dpdk
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Check if rte_cycles.h is included transitively or if NS_PER_S is defined locally
echo "=== Check what rte_ethdev.h includes ==="
rg "include.*rte_cycles" lib/ethdev/ --type h | head -5
echo ""
echo "=== Check all headers that might include rte_cycles ==="
rg "include.*rte_cycles" lib/ --type h --type c | head -10Repository: ovsrobot/dpdk
Length of output: 664
🏁 Script executed:
#!/bin/bash
# Check the function context to understand test intent better
echo "=== Get more context on the test function signature and comments ==="
sed -n '1300,1370p' app/test/test_pmd_pcap.c | head -80Repository: ovsrobot/dpdk
Length of output: 2352
🏁 Script executed:
#!/bin/bash
# Check if rte_cycles.h is included transitively through rte_mbuf.h or other included headers
echo "=== Check if rte_cycles.h is included in rte_mbuf.h ==="
grep -r "#include.*rte_cycles" lib/mbuf/rte_mbuf.h lib/ethdev/rte_ethdev.h 2>/dev/null || echo "Not directly included in those headers"
echo ""
echo "=== Check rte_mbuf.h includes ==="
head -30 lib/mbuf/rte_mbuf.h | grep "#include"Repository: ovsrobot/dpdk
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Check if the test file actually compiles/builds successfully with NS_PER_S
echo "=== Verify the test builds by looking for any meson/cmake config mentioning test_pmd_pcap ==="
find . -name "meson.build" -type f -exec grep -l "test_pmd_pcap" {} \; 2>/dev/null | head -3
echo ""
echo "=== Check test compilation context ==="
grep -A5 "test_pmd_pcap" app/test/meson.build 2>/dev/null | head -20Repository: ovsrobot/dpdk
Length of output: 744
🏁 Script executed:
#!/bin/bash
# Let's verify if NS_PER_S comes from a transitive include by checking what rte_eal_common.h includes
echo "=== Check if rte_eal.h includes rte_cycles.h ==="
rg "#include.*rte_cycles" lib/eal/include/ --type hRepository: ovsrobot/dpdk
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check if NS_PER_S is defined locally in the test file or comes from a header
echo "=== Search for NS_PER_S definitions in app/test/test_pmd_pcap.c ==="
grep -n "define NS_PER_S\|define NS_PER_SEC" app/test/test_pmd_pcap.c
echo ""
echo "=== Check if NS_PER_S is mentioned in test.h ==="
grep -r "NS_PER_S" app/test/test.h
echo ""
echo "=== Check full include chain of rte_mbuf.h to see if it indirectly includes rte_cycles.h ==="
head -80 lib/mbuf/rte_mbuf.hRepository: ovsrobot/dpdk
Length of output: 2450
🏁 Script executed:
#!/bin/bash
# Let's check if rte_eal.h brings in rte_cycles.h
echo "=== Check what's in rte_ethdev.h includes ==="
head -80 lib/ethdev/rte_ethdev.h | grep "#include"Repository: ovsrobot/dpdk
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Check test.h which is included
echo "=== Check what test.h includes ==="
head -100 app/test/test.h | grep -E "^#include"Repository: ovsrobot/dpdk
Length of output: 269
🏁 Script executed:
#!/bin/bash
# Check if rte_cycles.h is included transitively through rte_common.h or rte_test.h
echo "=== Check rte_common.h includes ==="
grep "#include" lib/eal/include/rte_common.h | head -20
echo ""
echo "=== Check rte_test.h includes ==="
grep "#include" lib/test/rte_test.h | head -20Repository: ovsrobot/dpdk
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Check if NS_PER_S is available in the generic/rte_cycles.h and trace back the include chain
echo "=== Check if rte_eal.h includes rte_cycles.h ==="
grep "#include.*rte_cycles" lib/eal/include/rte_eal.h
echo ""
echo "=== Check rte_eal.h to see what it includes ==="
head -40 lib/eal/include/rte_eal.h | grep "#include"
echo ""
echo "=== Check if test.h or included headers indirectly bring in rte_cycles ==="
for header in rte_test.h rte_common.h rte_hexdump.h rte_os_shim.h; do
echo "--- Checking $header ---"
grep -r "#include.*rte_cycles" lib/ | grep "$header" | head -2
doneRepository: ovsrobot/dpdk
Length of output: 545
🏁 Script executed:
#!/bin/bash
# Check if test file uses NS_PER_S and if it compiles - look for any compile warnings or errors in build logs
echo "=== Search for test compilation result or any indication it fails due to undefined NS_PER_S ==="
rg "NS_PER_S|undefined.*NS_PER_S" --type c --type h -C 2 app/test/Repository: ovsrobot/dpdk
Length of output: 2923
🏁 Script executed:
#!/bin/bash
# Check if other test files that use NS_PER_S explicitly include rte_cycles.h
echo "=== Check test_memcpy_perf.c includes ==="
grep -n "include.*rte_cycles\|#include" app/test/test_memcpy_perf.c | head -30
echo ""
echo "=== Check test_pcapng.c includes ==="
grep -n "include.*rte_cycles\|#include" app/test/test_pcapng.c | head -30Repository: ovsrobot/dpdk
Length of output: 854
🏁 Script executed:
#!/bin/bash
# Check if NS_PER_S is defined in packet_burst_generator.h (included in test_pmd_pcap.c)
echo "=== Check packet_burst_generator.h ==="
head -50 app/test/packet_burst_generator.h | grep -E "^#include|#define.*NS"Repository: ovsrobot/dpdk
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Let's verify by checking if there's a global definition or if headers chain includes rte_cycles
echo "=== Comprehensive check: find where NS_PER_S is ultimately coming from for test_pmd_pcap.c ==="
# Try to find if test.h or other headers indirectly include it through a chain
for header in rte_test rte_config rte_compat rte_os rte_hexdump rte_common rte_os_shim; do
echo "=== Checking for $header.h ==="
find lib -name "${header}.h" -type f 2>/dev/null | head -1 | xargs grep -l "rte_cycles" 2>/dev/null && echo "Found rte_cycles include in $header.h" || echo "No rte_cycles in $header.h"
doneRepository: ovsrobot/dpdk
Length of output: 591
🏁 Script executed:
#!/bin/bash
# Check what rte_time.h contains - test_pcapng.c uses it instead of rte_cycles.h
echo "=== Check rte_time.h for NS_PER_S ==="
grep -n "NS_PER_S\|define.*NS" lib/eal/include/rte_time.h | head -20
echo ""
echo "=== Check full rte_time.h includes ==="
head -40 lib/eal/include/rte_time.hRepository: ovsrobot/dpdk
Length of output: 1139
🏁 Script executed:
#!/bin/bash
# Check if test_pmd_pcap.c should have rte_cycles.h or rte_time.h like the other test files
echo "=== Check if test_pmd_pcap.c should include rte_time.h or rte_cycles.h ==="
# See what other test files using NS_PER_S import
echo "Files using NS_PER_S:"
grep -l "NS_PER_S" app/test/*.c | xargs -I {} sh -c 'echo "=== {} ==="; head -30 {} | grep "#include.*rte_"'Repository: ovsrobot/dpdk
Length of output: 865
🏁 Script executed:
#!/bin/bash
# Check if rte_pcapng.h includes rte_cycles.h (since test_pcapng uses NS_PER_S but doesn't include rte_cycles)
echo "=== Check rte_pcapng.h includes ==="
head -50 lib/pcapng/rte_pcapng.h | grep "#include"
echo ""
echo "=== Check if NS_PER_S is defined in rte_pcapng.h ==="
grep -n "NS_PER_S\|NSEC_PER_SEC" lib/pcapng/rte_pcapng.h
echo ""
echo "=== Check if rte_mbuf.h or rte_ethdev.h include rte_cycles.h ==="
for file in lib/mbuf/rte_mbuf.h lib/ethdev/rte_ethdev.h; do
echo "=== $file ==="
grep "rte_cycles" "$file" || echo "No rte_cycles"
doneRepository: ovsrobot/dpdk
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Let's check if test_pcapng.c actually builds or if it's also missing the include
echo "=== Check if test_pcapng.c's meson.build has special dependencies ==="
grep -A2 -B2 "test_pcapng" app/test/meson.build
echo ""
echo "=== Search for where NS_PER_S might be defined in pcapng library ==="
grep -r "define NS_PER_S" lib/pcapng/Repository: ovsrobot/dpdk
Length of output: 581
🏁 Script executed:
#!/bin/bash
# Final check: Let's see if rte_pcapng.c (implementation) includes rte_cycles.h
echo "=== Check lib/pcapng/rte_pcapng.c includes ==="
head -30 lib/pcapng/rte_pcapng.c | grep "#include"
echo ""
echo "=== Check if any library included by test_pmd_pcap.c transitively includes rte_cycles ==="
# Check rte_net.h since it might be common
grep -r "#include.*rte_cycles" lib/net/ --type h 2>/dev/null | head -5
echo ""
echo "=== Final verification: Does NS_PER_S exist in any of test_pmd_pcap.c's direct includes? ==="
for header in rte_ethdev rte_bus_vdev rte_mbuf rte_mbuf_dyn rte_mempool rte_ether rte_ip rte_udp; do
echo "=== Checking ${header}.h ==="
find lib -name "${header}.h" -type f 2>/dev/null | xargs grep -l "NS_PER_S" 2>/dev/null && echo "Found NS_PER_S!" || echo "Not found"
doneRepository: ovsrobot/dpdk
Length of output: 1092
Add missing #include <rte_cycles.h> for NS_PER_S definition.
The test uses NS_PER_S at line 1375, but this macro is defined in lib/eal/include/generic/rte_cycles.h, which is not included in this file. Other test files (e.g., test_memcpy_perf.c) explicitly include rte_cycles.h before using NS_PER_S. The current includes do not transitively provide this macro.
Additionally, the timestamp comparison at lines 1378-1380 only prints a warning on mismatch rather than failing the test. Consider whether mismatched timestamps should trigger a TEST_ASSERT instead of a silent warning, especially since the test explicitly validates timestamp accuracy.
🤖 Prompt for AI Agents
In @app/test/test_pmd_pcap.c around lines 1367 - 1396, The test uses NS_PER_S
but never includes <rte_cycles.h>, so add a header include for rte_cycles.h near
the other includes to bring in the NS_PER_S definition; also replace the silent
printf warning that reports timestamp mismatch (the message printed when ts !=
expected) with a failing assertion using TEST_ASSERT (or augment the existing
check to call TEST_ASSERT(ts == expected, "... expected=%"PRIu64"
actual=%"PRIu64"", expected, ts)) so timestamp mismatches fail the test rather
than only logging a warning.
| int osdep_iface_mtu_set(int ifindex, uint16_t mtu) | ||
| { | ||
| char ifname[IFNAMSIZ]; | ||
|
|
||
| if (if_indextoname(ifindex, ifname) == NULL) | ||
| return -errno; | ||
|
|
||
| int s = socket(AF_INET, SOCK_DGRAM, 0); | ||
| if (s < 0) | ||
| return -errno; | ||
|
|
||
| struct ifreq ifr = { 0 }; | ||
| if (s < 0) | ||
| return -EINVAL; | ||
|
|
||
| strlcpy(ifr.ifr_name, ifname, IFNAMSIZ); | ||
| ifr.ifr_mtu = mtu; | ||
|
|
||
| int ret = ioctl(s, SIOCSIFMTU, &ifr); | ||
| close(s); | ||
| return (ret < 0) ? -errno : 0; | ||
| } |
There was a problem hiding this comment.
Dead code: redundant socket check.
Lines 96-97 contain a redundant if (s < 0) check that can never be true because the function already returns at line 93 if socket creation fails. This appears to be a copy-paste error.
Proposed fix
int osdep_iface_mtu_set(int ifindex, uint16_t mtu)
{
char ifname[IFNAMSIZ];
if (if_indextoname(ifindex, ifname) == NULL)
return -errno;
int s = socket(AF_INET, SOCK_DGRAM, 0);
if (s < 0)
return -errno;
struct ifreq ifr = { 0 };
- if (s < 0)
- return -EINVAL;
-
strlcpy(ifr.ifr_name, ifname, IFNAMSIZ);
ifr.ifr_mtu = mtu;
int ret = ioctl(s, SIOCSIFMTU, &ifr);
close(s);
return (ret < 0) ? -errno : 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int osdep_iface_mtu_set(int ifindex, uint16_t mtu) | |
| { | |
| char ifname[IFNAMSIZ]; | |
| if (if_indextoname(ifindex, ifname) == NULL) | |
| return -errno; | |
| int s = socket(AF_INET, SOCK_DGRAM, 0); | |
| if (s < 0) | |
| return -errno; | |
| struct ifreq ifr = { 0 }; | |
| if (s < 0) | |
| return -EINVAL; | |
| strlcpy(ifr.ifr_name, ifname, IFNAMSIZ); | |
| ifr.ifr_mtu = mtu; | |
| int ret = ioctl(s, SIOCSIFMTU, &ifr); | |
| close(s); | |
| return (ret < 0) ? -errno : 0; | |
| } | |
| int osdep_iface_mtu_set(int ifindex, uint16_t mtu) | |
| { | |
| char ifname[IFNAMSIZ]; | |
| if (if_indextoname(ifindex, ifname) == NULL) | |
| return -errno; | |
| int s = socket(AF_INET, SOCK_DGRAM, 0); | |
| if (s < 0) | |
| return -errno; | |
| struct ifreq ifr = { 0 }; | |
| strlcpy(ifr.ifr_name, ifname, IFNAMSIZ); | |
| ifr.ifr_mtu = mtu; | |
| int ret = ioctl(s, SIOCSIFMTU, &ifr); | |
| close(s); | |
| return (ret < 0) ? -errno : 0; | |
| } |
🤖 Prompt for AI Agents
In @drivers/net/pcap/pcap_osdep_freebsd.c around lines 84 - 105, The redundant
socket error check in osdep_iface_mtu_set should be removed: delete the second
"if (s < 0) return -EINVAL;" (the earlier socket() failure already returns),
leaving the socket variable use intact; ensure struct ifreq ifr remains
zero-initialized (struct ifreq ifr = { 0 };), copy the interface name with
strlcpy(ifr.ifr_name, ifname, IFNAMSIZ) and set ifr.ifr_mtu = mtu before calling
ioctl and closing the socket.
|
|
||
| int osdep_iface_mtu_set(int index __rte_unused, uint16_t mtu __rte_unused) | ||
| { | ||
| return -ENOTSUP; | ||
| } |
There was a problem hiding this comment.
Missing osdep_iface_mac_set implementation for Windows.
The header pcap_osdep.h declares osdep_iface_mac_set() (line 17), and implementations exist in pcap_osdep_linux.c and pcap_osdep_freebsd.c, but this Windows file lacks a corresponding stub. This will cause a linker error when building on Windows.
Proposed fix
int osdep_iface_mtu_set(int index __rte_unused, uint16_t mtu __rte_unused)
{
return -ENOTSUP;
}
+
+int osdep_iface_mac_set(int index __rte_unused,
+ const struct rte_ether_addr *mac __rte_unused)
+{
+ return -ENOTSUP;
+}🤖 Prompt for AI Agents
In @drivers/net/pcap/pcap_osdep_windows.c around lines 119 - 123, Add the
missing osdep_iface_mac_set function to match the declaration in pcap_osdep.h:
implement a Windows stub named osdep_iface_mac_set with the same signature as
declared (use the same parameter names/unused annotations as other Windows
stubs) and return -ENOTSUP, mirroring osdep_iface_mtu_set's behavior so the
symbol is defined and the linker error is resolved.
2 participants
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=37002"
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.