Improve virtual_network.iface_bridge cases to adapt to new testing tool#6816
Open
yanglei-rh wants to merge 1 commit intoautotest:masterfrom
Open
Improve virtual_network.iface_bridge cases to adapt to new testing tool#6816yanglei-rh wants to merge 1 commit intoautotest:masterfrom
yanglei-rh wants to merge 1 commit intoautotest:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR modifies virtual_network iface_bridge config and test behavior: new config options were added and some hardcoded iface_source entries removed. The test now prefers Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
116-116: Normalizenetdstto a single bridge token before use.If
netdstis comma-separated, this can produce an invalid bridge name downstream. Prefer taking the first entry explicitly.Suggested adjustment
- bridge_name = params.get("netdst") or params.get("bridge_name", "test_br0") + netdst = params.get("netdst", "") + bridge_name = netdst.split(",")[0] if netdst else params.get("bridge_name", "test_br0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/iface_bridge.py` at line 116, Normalize the netdst value before assigning bridge_name: if params.get("netdst") returns a comma-separated string, split it on ',' and take the first token (trim whitespace) as the bridge token, otherwise fall back to params.get("bridge_name", "test_br0"); update the bridge_name assignment (the variable bridge_name and its use of params.get("netdst") / params.get("bridge_name")) to perform this normalization so downstream code receives a single valid bridge name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virtual_network/iface_bridge.py`:
- Around line 174-179: The current logic cancels the test whenever
libvirt.check_iface(...) reports the bridge exists, which wrongly affects
non-self-managed bridges; change the condition so that test.cancel("The bridge
%s already exist" % bridge_name) only runs when bridge_name == "test_br0" and
check_iface(...) returns true; keep the existing create path using
create_linux_bridge_tmux(bridge_name, iface_name) and its failure handling for
the test_br0 branch (i.e., call test.fail on non-zero s), but do not cancel for
other bridge names if they already exist.
- Line 128: Replace the unsafe eval call that sets iface_source with
ast.literal_eval: parse params.get("iface_source", "{'bridge':'%s'}" %
bridge_name) using ast.literal_eval, then validate the result is a dict (and
optionally that it contains the expected 'bridge' key) before using it; update
the code that references iface_source accordingly (symbols: iface_source,
params.get, bridge_name, ast.literal_eval).
---
Nitpick comments:
In `@libvirt/tests/src/virtual_network/iface_bridge.py`:
- Line 116: Normalize the netdst value before assigning bridge_name: if
params.get("netdst") returns a comma-separated string, split it on ',' and take
the first token (trim whitespace) as the bridge token, otherwise fall back to
params.get("bridge_name", "test_br0"); update the bridge_name assignment (the
variable bridge_name and its use of params.get("netdst") /
params.get("bridge_name")) to perform this normalization so downstream code
receives a single valid bridge name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d27283f-ccd5-441e-a394-4c571d2ead03
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_bridge.cfglibvirt/tests/src/virtual_network/iface_bridge.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22326
branch
from
e91280e to
849ac36
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/iface_bridge.py (1)
128-128:⚠️ Potential issue | 🟠 MajorReplace unsafe
evalforiface_sourceparsing.Line 128 executes arbitrary input from params. Use
ast.literal_evalplus a dict type check before use.Suggested fix
- iface_source = eval(params.get("iface_source", "{'bridge':'%s'}" % bridge_name)) + try: + iface_source = ast.literal_eval( + params.get("iface_source", "{'bridge':'%s'}" % bridge_name) + ) + except (ValueError, SyntaxError) as err: + test.cancel("Invalid iface_source literal: %s" % err) + if not isinstance(iface_source, dict): + test.cancel("Invalid iface_source, expected dict-like literal")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/iface_bridge.py` at line 128, Replace the unsafe eval usage that sets iface_source by parsing params.get("iface_source", ...) with ast.literal_eval and validate the result is a dict: call ast.literal_eval on the string value for iface_source (fall back to the existing default string when missing), catch ValueError/SyntaxError/TypeError, and if successful ensure isinstance(iface_source, dict) before using it; if parsing fails or the type check fails, log/raise an appropriate error or revert to the default dict so code using iface_source (the variable set where the current line uses eval) never executes untrusted code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libvirt/tests/src/virtual_network/iface_bridge.py`:
- Line 128: Replace the unsafe eval usage that sets iface_source by parsing
params.get("iface_source", ...) with ast.literal_eval and validate the result is
a dict: call ast.literal_eval on the string value for iface_source (fall back to
the existing default string when missing), catch
ValueError/SyntaxError/TypeError, and if successful ensure
isinstance(iface_source, dict) before using it; if parsing fails or the type
check fails, log/raise an appropriate error or revert to the default dict so
code using iface_source (the variable set where the current line uses eval)
never executes untrusted code.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6758631b-75a4-4709-8a78-5ceb0c98ed54
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_bridge.cfglibvirt/tests/src/virtual_network/iface_bridge.py
Contributor
Author
|
Test pass. |
YongxueHong
reviewed
Mar 10, 2026
1.Add some parameters to allow the framework provide guest xml 2.Add "only Linux" to limit guest type 3.Add a check about host bridge type Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22326
branch
from
849ac36 to
26c0549
Compare
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
2 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.
1.Add some parameters to allow the framework provide guest xml
2.Add "only Linux" to limit guest type
3.Add a check about host bridge type
ID: LIBVIRTAT-22326
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit