[PWCI] "[ovs-dev] checkpatch: Use the AI suggested group syntax."#5
Open
[PWCI] "[ovs-dev] checkpatch: Use the AI suggested group syntax."#5
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>
Sourcery AI suggests: Replace m.group(x) with m[x] for re.Match objects Suggested-by: SourceryAI <hello@sourcery.ai> 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 enhances error-path cleanup in DPDK flow parsing by freeing allocated buffers on failure, corrects the offload_data lookup target in flow destruction, and adopts AI-suggested regex group syntax in the checkpatch utility. Sequence diagram for improved error cleanup in DPDK flow parsingsequenceDiagram
participant Netdev as netdev
participant Parser as parse_flow_match
participant VLOG as VLOG_WARN_RL
participant Memory as Memory
Netdev->>Parser: parse_flow_match(...)
Parser->>Parser: Check IPv4/IPv6 frag
alt Unknown frag value
Parser->>VLOG: VLOG_WARN_RL(...)
Parser->>Memory: free(spec/frag_spec)
Parser->>Memory: free(mask/frag_mask)
Parser-->>Netdev: return -1
end
Class diagram for updated offload_data lookup in flow destructionclassDiagram
class netdev_offload_dpdk_flow_destroy {
+unsigned int tid
+data: netdev_offload_dpdk_data
+physdev: netdev
+rte_flow_counters[]
+VLOG_DBG_RL(...)
}
class netdev {
+hw_info: hw_info
}
class physdev {
+hw_info: hw_info
}
class hw_info {
+offload_data
}
class netdev_offload_dpdk_data {
+rte_flow_counters[]
}
netdev_offload_dpdk_flow_destroy --> physdev : uses
physdev --> hw_info : has
hw_info --> netdev_offload_dpdk_data : offload_data
Class diagram for checkpatch.py spelling check group syntax updateclassDiagram
class checkpatch {
+check_spelling(line, comment_type)
}
checkpatch : check_spelling uses re.search
checkpatch : check_spelling uses matched[1] for group access
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughUpdates include: adding an author entry; fixing memory leaks and correcting offload data lookup in DPDK offload flow handling; and a minor regex group access change in a Python check script. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant FlowMgr as Flow Manager
participant PhysDev as physdev
Note over FlowMgr,PhysDev: netdev_offload_dpdk_flow_destroy()
Caller->>FlowMgr: destroy(flow)
FlowMgr->>PhysDev: access hw_info.offload_data
Note right of PhysDev: Use physdev->hw_info.offload_data<br/>(not netdev->hw_info.offload_data)
FlowMgr->>FlowMgr: decrement counters, log
FlowMgr-->>Caller: return
sequenceDiagram
autonumber
participant Parser as parse_flow_match
participant Alloc as spec/mask alloc
Note over Parser: Fragmentation parsing (IPv4/IPv6)
Parser->>Alloc: allocate spec/mask (frag_spec/frag_mask)
Parser->>Parser: detect unknown fragmentation
alt Error: unknown fragmentation
Parser->>Alloc: free allocated spec/mask
Parser-->>Parser: return -1
else Known/handled
Parser-->>Parser: continue
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `utilities/checkpatch.py:407` </location>
<code_context>
words = ""
matched = re.search(r'#([^!].*)$', line)
if matched:
- words = matched.group(1)
+ words = matched[1]
else:
words = line.replace("'''", '').replace('"""', '').strip()
</code_context>
<issue_to_address>
Using 'matched[1]' instead of 'matched.group(1)' may cause issues if 'matched' is not a Match object.
'matched[1]' will raise a TypeError if 'matched' is a re.Match object. Use 'matched.group(1)' unless you are certain 'matched' is a tuple or similar.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| words = "" | ||
| matched = re.search(r'#([^!].*)$', line) | ||
| if matched: | ||
| words = matched.group(1) |
There was a problem hiding this comment.
issue (bug_risk): Using 'matched[1]' instead of 'matched.group(1)' may cause issues if 'matched' is not a Match object.
'matched[1]' will raise a TypeError if 'matched' is a re.Match object. Use 'matched.group(1)' unless you are certain 'matched' is a tuple or similar.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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/checkpatch.py(1 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). (21)
- GitHub Check: linux clang address,undefined check-kernel -100
- GitHub Check: linux clang dpdk-shared --enable-shared
- GitHub Check: linux gcc dpdk --enable-shared
- GitHub Check: linux clang check check-dpdk dpdk
- GitHub Check: linux clang test --enable-shared
- GitHub Check: linux gcc test --enable-shared
- GitHub Check: linux gcc --disable-ssl
- GitHub Check: linux clang test -ljemalloc
- GitHub Check: linux clang c99
- GitHub Check: linux gcc --enable-afxdp
- GitHub Check: linux clang --disable-ssl
- GitHub Check: linux gcc test -ljemalloc
- GitHub Check: linux gcc test
- GitHub Check: linux clang address,undefined test
- GitHub Check: clang-analyze-cache
- GitHub Check: freebsd_build COMPILER:gcc freebsd_instance:family/freebsd-14-3-snap
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-13-5-snap
- GitHub Check: linux deb no dpdk
- GitHub Check: osx clang --disable-ssl
- GitHub Check: linux rpm fedora
- GitHub Check: build oss-fuzz fuzzers
🔇 Additional comments (3)
AUTHORS.rst (1)
298-299: Add Linda Wang to AUTHORS – LGTMAlphabetical placement under “Linda Sun” looks correct. No further action.
lib/netdev-offload-dpdk.c (2)
1527-1532: Good fix: free IPv4 spec/mask on unknown frag pathPrevents leaks when returning early before the pattern is added. No further issues spotted here.
1612-1617: Free IPv6 frag_spec/frag_mask on unknown frag – check ownership of base spec/maskThis correctly frees the frag ext allocations. Confirm that the base IPv6
spec/maskwere already added topatternsbefore this branch (they are), so they’ll be freed byfree_flow_patterns()on error. Avoid freeing them here to prevent double-free.
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.
🛠️ Refactor suggestion
Use physdev offload_data (correct) and guard against NULL to avoid crash
Switching to physdev->hw_info.offload_data matches where counters are maintained. Add a NULL check to avoid a rare NPE during teardown races.
- data = (struct netdev_offload_dpdk_data *)
- ovsrcu_get(void *, &physdev->hw_info.offload_data);
- data->rte_flow_counters[tid]--;
+ data = (struct netdev_offload_dpdk_data *)
+ ovsrcu_get(void *, &physdev->hw_info.offload_data);
+ if (data) {
+ data->rte_flow_counters[tid]--;
+ } else {
+ VLOG_DBG_RL(&rl, "%s: offload_data missing on physdev while destroying flow 0x%"PRIxPTR,
+ netdev_get_name(physdev), (intptr_t) rte_flow);
+ }📝 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
| 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]--; | |
| data = (struct netdev_offload_dpdk_data *) | |
| ovsrcu_get(void *, &physdev->hw_info.offload_data); | |
| if (data) { | |
| data->rte_flow_counters[tid]--; | |
| } else { | |
| VLOG_DBG_RL(&rl, "%s: offload_data missing on physdev while destroying flow 0x%"PRIxPTR, | |
| netdev_get_name(physdev), (intptr_t) rte_flow); | |
| } |
🤖 Prompt for AI Agents
In lib/netdev-offload-dpdk.c around lines 2381-2384, the code reads
physdev->hw_info.offload_data via ovsrcu_get and then unconditionally
dereferences it to decrement a counter; add a NULL guard to avoid a potential
NULL-pointer crash during teardown races. Change the sequence to retrieve the
offload_data into a local pointer, check if that pointer is non-NULL, and only
then decrement data->rte_flow_counters[tid] (otherwise skip the decrement).
Comment on lines
405
to
409
| matched = re.search(r'#([^!].*)$', line) | ||
| if matched: | ||
| words = matched.group(1) | ||
| words = matched[1] | ||
| else: | ||
| words = line.replace("'''", '').replace('"""', '').strip() |
There was a problem hiding this comment.
Use Match.group(1) instead of subscription for portability
matched[1] isn’t supported on older Python versions commonly used with OVS; the rest of this file already uses .group(n). This change can raise “'re.Match' object is not subscriptable”.
Apply:
- if matched:
- words = matched[1]
+ if matched:
+ words = matched.group(1)📝 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
| matched = re.search(r'#([^!].*)$', line) | |
| if matched: | |
| words = matched.group(1) | |
| words = matched[1] | |
| else: | |
| words = line.replace("'''", '').replace('"""', '').strip() | |
| matched = re.search(r'#([^!].*)$', line) | |
| if matched: | |
| words = matched.group(1) | |
| else: | |
| words = line.replace("'''", '').replace('"""', '').strip() |
🤖 Prompt for AI Agents
In utilities/checkpatch.py around lines 405 to 409, the code uses matched[1] to
extract the regex capture which fails on older Python versions; replace that
subscription with matched.group(1) to match the rest of the file and avoid
"'re.Match' object is not subscriptable" errors—i.e., when matched is truthy
call matched.group(1) to assign words.
There was a problem hiding this comment.
Since which python version is it supported?
There was a problem hiding this comment.
.
So the subscriptable Match object syntax (matched[1]) is supported starting from Python 3.6, but for maximum compatibility with older Python versions commonly used in systems like OVS, using matched.group(1) is the safer approach.
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
5 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] checkpatch: Use the AI suggested group syntax.".
See "http://patchwork.ozlabs.org/project/openvswitch/list/?series=471914" for details.
Summary by Sourcery
Fix memory management and data retrieval bugs in the DPDK netdev offload code and update checkpatch script to use the new regex group syntax
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Documentation
Chores