[PWCI] "[ovs-dev] ovs-tcpdump: Update the usage information to be more helpful."#6
Open
[PWCI] "[ovs-dev] ovs-tcpdump: Update the usage information to be more helpful."#6
Conversation
For vport devices hardware offload counters were being incremented on the physical device but decremented on the vport, causing incorrect statistics that didn't return to zero when flows are removed. Fix by using physdev instead of netdev for counter decrements to match the increment behavior. Fixes: 0e6366c ("netdev-offload-dpdk: Implement hw-offload statistics read.") Signed-off-by: Linda Wang <linda.wang@jaguarmicro.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
The error paths should release any allocated resources that are not added to the patterns. Found by Coverity. Fixes: c1a5d0e ("netdev-offload-dpdk: Support IPv4 fragmentation types.") Fixes: 1318929 ("netdev-offload-dpdk: Support IPv6 fragmentation types.") Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> Reviewed-by: David Marchand <david.marchand@redhat.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
There is some confusion about which options can be passed to ovs-tcpdump when gathering data. The interpretation sometimes is that ovs-tcpdump only accepts a specific small set of options and discards other options. This leads to questions such as, "How can I use '-C' or set the snaplen with ovs-tcpdump like tcpdump allows?" To correct this, print the dump_cmd help after the ovs-tcpdump options. As part of this rework, we delay printing the usage data until after all arguments are processed, which allows setting the dumpcmd as well for the --help option. Signed-off-by: Aaron Conole <aconole@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes memory leaks in DPDK flow parsing, corrects an RCU data retrieval bug in flow teardown, and refines the ovs-tcpdump usage text (plus updates the AUTHORS list). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughUpdates AUTHORS.rst to add an author entry. In lib/netdev-offload-dpdk.c, adds memory frees on error paths and switches counter updates to use physdev. In utilities/ovs-tcpdump.in, makes usage dynamic by invoking the underlying tcpdump’s help, adjusts help flow, and adds a parameterized usage(tcpdump_prog). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant O as ovs-tcpdump
participant S as System PATH
participant T as tcpdump
rect rgb(245,248,255)
note right of U: Request help (-h/--help)
U->>O: Invoke with -h/--help
O->>S: Locate tcpdump program
alt tcpdump found
O->>T: Run `tcpdump --help`
T-->>O: Help text (stdout)
O-->>U: Print ovs-tcpdump usage + embedded tcpdump help
else tcpdump missing
O-->>U: Error: tcpdump not found
end
end
rect rgb(245,255,245)
note right of U: Normal run
U->>O: Invoke with interface/args
O->>S: Locate tcpdump program
alt validation ok
O->>T: Execute tcpdump with args
T-->>O: Stream output
O-->>U: Forward output
else validation error
O-->>U: Print error/usage as applicable
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
utilities/ovs-tcpdump.in (1)
500-506: Show usage even if tcpdump is missing.Currently, requesting “--help” fails if dump_cmd isn’t in PATH. Consider printing wrapper usage anyway and annotating that underlying help isn’t available.
- if not py_which(dump_cmd): - print("Error: unable to execute '%s' (check PATH)" % dump_cmd) - sys.exit(1) - - if print_usage: - usage(dump_cmd) + if print_usage: + # usage() handles missing tcpdump and will note it. + usage(dump_cmd) + + if not py_which(dump_cmd): + print("Error: unable to execute '%s' (check PATH)" % dump_cmd) + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
AUTHORS.rst(1 hunks)lib/netdev-offload-dpdk.c(3 hunks)utilities/ovs-tcpdump.in(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: linux clang dpdk --enable-shared
- GitHub Check: linux gcc test -ljemalloc
- GitHub Check: linux gcc dpdk --enable-shared
- GitHub Check: linux gcc check check-dpdk dpdk
- GitHub Check: linux clang --enable-afxdp
- GitHub Check: linux clang test --enable-shared
- GitHub Check: linux clang check check-dpdk dpdk
- GitHub Check: linux clang --disable-ssl
- GitHub Check: linux gcc --disable-ssl
- GitHub Check: linux clang test -ljemalloc
- GitHub Check: linux gcc test --enable-shared
- GitHub Check: linux clang address,undefined test
- GitHub Check: linux clang c99
- GitHub Check: linux gcc test
- GitHub Check: linux gcc c99
- GitHub Check: clang-analyze-cache
- GitHub Check: linux deb no dpdk
- GitHub Check: linux rpm fedora
- GitHub Check: osx clang --disable-ssl
- GitHub Check: build oss-fuzz fuzzers
🔇 Additional comments (6)
AUTHORS.rst (1)
298-299: LGTM: author entry looks correct and alphabetized.Placement and formatting match surrounding entries.
lib/netdev-offload-dpdk.c (2)
1527-1531: Good fix: free IPv4 spec/mask on error path.Prevents leaks when encountering unknown IPv4 frag flags before the item is added to patterns.
1612-1616: Good fix: free IPv6 frag spec/mask on error path.Prevents leaks when encountering unknown IPv6 frag conditions.
utilities/ovs-tcpdump.in (3)
463-463: Initialize print_usage: good call.Enables deferred usage printing without short-circuiting argument parsing.
470-471: Defer help printing instead of exiting early: good UX.Keeps validation order predictable and allows including tcpdump’s help text.
507-510: Interface check after help: good.Prevents spurious errors when only requesting usage.
Comment on lines
2381
to
2384
| data = (struct netdev_offload_dpdk_data *) | ||
| ovsrcu_get(void *, &netdev->hw_info.offload_data); | ||
| ovsrcu_get(void *, &physdev->hw_info.offload_data); | ||
| data->rte_flow_counters[tid]--; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use physdev for counter decrement to match increment site.
Switching offload_data source to physdev keeps rte_flow_counters consistent with creation (which increments on physdev). Consider adding a brief comment here to document the invariant.
Run to verify that all inc/dec sites consistently use physdev:
🏁 Script executed:
#!/bin/bash
rg -n -C2 --type=c 'rte_flow_counters\[[^]]+\]\+\+|rte_flow_counters\[[^]]+\]\-\-' lib/netdev-offload-dpdk.cLength of output: 558
Synchronize counter sites to use physdev
In lib/netdev-offload-dpdk.c at the increment site (around line 932), you’re still doing:
ovsrcu_get(void *, &netdev->hw_info.offload_data);but the delete path now uses physdev->hw_info.offload_data. Change the increment to use physdev so both ++ and -- touch the same counter array.
Comment on lines
+128
to
+131
| def usage(tcpdump_prog='tcpdump'): | ||
| tcpd_help = _doexec(*([tcpdump_prog, '--help'])) | ||
| tcpd_help.wait() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make help capture robust (stderr, fallback to -h, avoid deadlocks).
Some tcpdump builds print help to stderr or only support “-h”. Use subprocess.run with stderr=STDOUT and fallback to “-h”, and avoid wait()+read() sequencing.
Apply this diff to usage():
-def usage(tcpdump_prog='tcpdump'):
- tcpd_help = _doexec(*([tcpdump_prog, '--help']))
- tcpd_help.wait()
+def usage(tcpdump_prog='tcpdump'):
+ # Try -h first, then --help; capture stdout+stderr; tolerate missing binary.
+ help_text = ""
+ for args in ((tcpdump_prog, '-h'), (tcpdump_prog, '--help')):
+ try:
+ proc = subprocess.run(args, stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT, text=True)
+ help_text = (proc.stdout or "").strip()
+ if help_text:
+ break
+ except FileNotFoundError:
+ help_text = f"(unable to execute '{tcpdump_prog}' — only ovs-tcpdump options shown)"
+ breakCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In utilities/ovs-tcpdump.in around lines 128 to 131, the usage() function
currently calls _doexec(... '--help') and then wait()+read(), which can deadlock
and misses help printed to stderr or when only '-h' is supported; change it to
use subprocess.run to invoke tcpdump with '--help' capturing combined output
(stderr=subprocess.STDOUT) and text=True, and if that returns a non-zero exit or
no output, retry with '-h'; remove wait()/read() sequencing and use the
completed process' stdout as the help text for further processing.
Comment on lines
+150
to
154
| The following are the '%(tcpdump_prog)s' options: | ||
| %(tcpdump_help)s | ||
| """ % {'prog': sys.argv[0], 'tcpdump_prog': tcpdump_prog, | ||
| 'tcpdump_help': tcpd_help.stdout.read().decode('utf-8')}) | ||
| sys.exit(0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Print captured help safely.
Use the captured help_text string; avoids decode errors and works with the revised usage() above.
-The following are the '%(tcpdump_prog)s' options:
-%(tcpdump_help)s
-""" % {'prog': sys.argv[0], 'tcpdump_prog': tcpdump_prog,
- 'tcpdump_help': tcpd_help.stdout.read().decode('utf-8')})
+The following are the '%(tcpdump_prog)s' options:
+%(tcpdump_help)s
+""" % {'prog': sys.argv[0], 'tcpdump_prog': tcpdump_prog,
+ 'tcpdump_help': help_text})📝 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.
Suggested change
| The following are the '%(tcpdump_prog)s' options: | |
| %(tcpdump_help)s | |
| """ % {'prog': sys.argv[0], 'tcpdump_prog': tcpdump_prog, | |
| 'tcpdump_help': tcpd_help.stdout.read().decode('utf-8')}) | |
| sys.exit(0) | |
| The following are the '%(tcpdump_prog)s' options: | |
| %(tcpdump_help)s | |
| """ % {'prog': sys.argv[0], 'tcpdump_prog': tcpdump_prog, | |
| 'tcpdump_help': help_text}) | |
| sys.exit(0) |
🤖 Prompt for AI Agents
In utilities/ovs-tcpdump.in around lines 150 to 154, the usage string is
currently inserting tcpd_help.stdout.read().decode('utf-8') which can cause
decode errors; change the interpolation to use the already-captured help_text
string (e.g., replace tcpd_help.stdout.read().decode('utf-8') with help_text) so
the usage() prints the safe, pre-decoded help_text and update the dict passed to
% accordingly.
ovsrobot
pushed a commit
that referenced
this pull request
Sep 19, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
ovsrobot
pushed a commit
that referenced
this pull request
Oct 9, 2025
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: This is an auto submission for "[ovs-dev] ovs-tcpdump: Update the usage information to be more helpful.".
See "http://patchwork.ozlabs.org/project/openvswitch/list/?series=472073" for details.
Summary by Sourcery
Fix memory leaks in DPDK flow parsing, correct offload_data lookup for DPDK flow destruction, and refresh AUTHORS and ovs-tcpdump usage information
Bug Fixes:
Documentation:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation