iface_attach_detach: Support new libvirt error message for boot order conflicts#6802
iface_attach_detach: Support new libvirt error message for boot order conflicts#6802BulaYoungR wants to merge 1 commit intoautotest:masterfrom
Conversation
WalkthroughChanges across the virtual network iface_attach_detach test add minor code refactors and formatting consistency, update test parameter handling, and adjust configuration error patterns. In the test code: removed Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
Before fix After fix |
![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
🤖 Fix all issues with AI agents
In `@libvirt/tests/src/virtual_network/iface_attach_detach.py`:
- Line 51: Remove the unused import of the re module in the
iface_attach_detach.py test file: delete the standalone "import re" statement
(it’s not referenced anywhere; libvirt.check_result() receives regex strings
itself). Check the top of the file where imports are declared to ensure no other
references to re remain and run tests to confirm no breakage.
- Around line 52-58: The code creates error_messages = error_msg when error_msg
is a list, then calls error_messages.extend(...), which mutates the original
error_msg; change the assignment to make a copy (e.g., use list(error_msg) or
error_msg.copy()) so error_messages is a new list before extending with
alternative_patterns; keep the rest (alternative_patterns and extend) the same
and only modify the creation of error_messages to avoid mutating the passed-in
error_msg.
| # Old: "per-device boot elements cannot be used together with os/boot elements" | ||
| # New: "boot order X is already used by another device" | ||
| if status_error and error_msg: | ||
| import re |
There was a problem hiding this comment.
Remove unused import.
The re module is imported but never used. The regex patterns are passed as strings to libvirt.check_result(), which handles the matching internally.
Proposed fix
if status_error and error_msg:
- import re
error_messages = error_msg if isinstance(error_msg, list) else [error_msg]📝 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.
| import re | |
| if status_error and error_msg: | |
| error_messages = error_msg if isinstance(error_msg, list) else [error_msg] |
🤖 Prompt for AI Agents
In `@libvirt/tests/src/virtual_network/iface_attach_detach.py` at line 51, Remove
the unused import of the re module in the iface_attach_detach.py test file:
delete the standalone "import re" statement (it’s not referenced anywhere;
libvirt.check_result() receives regex strings itself). Check the top of the file
where imports are declared to ensure no other references to re remain and run
tests to confirm no breakage.
| error_messages = error_msg if isinstance(error_msg, list) else [error_msg] | ||
| # Add known alternative patterns for boot order conflicts | ||
| alternative_patterns = [ | ||
| "boot order .* is already used by another device", | ||
| "per-device boot elements cannot be used together with os/boot elements" | ||
| ] | ||
| error_messages.extend(alternative_patterns) |
There was a problem hiding this comment.
Potential list mutation issue.
If error_msg is already a list, line 52 assigns a reference to the same list object (not a copy). The subsequent extend() on line 58 then mutates the original error_msg, which could cause unexpected side effects if the parameter is examined or reused elsewhere.
Proposed fix to avoid mutating the original
- error_messages = error_msg if isinstance(error_msg, list) else [error_msg]
+ error_messages = list(error_msg) if isinstance(error_msg, list) else [error_msg]📝 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.
| error_messages = error_msg if isinstance(error_msg, list) else [error_msg] | |
| # Add known alternative patterns for boot order conflicts | |
| alternative_patterns = [ | |
| "boot order .* is already used by another device", | |
| "per-device boot elements cannot be used together with os/boot elements" | |
| ] | |
| error_messages.extend(alternative_patterns) | |
| error_messages = list(error_msg) if isinstance(error_msg, list) else [error_msg] | |
| # Add known alternative patterns for boot order conflicts | |
| alternative_patterns = [ | |
| "boot order .* is already used by another device", | |
| "per-device boot elements cannot be used together with os/boot elements" | |
| ] | |
| error_messages.extend(alternative_patterns) |
🤖 Prompt for AI Agents
In `@libvirt/tests/src/virtual_network/iface_attach_detach.py` around lines 52 -
58, The code creates error_messages = error_msg when error_msg is a list, then
calls error_messages.extend(...), which mutates the original error_msg; change
the assignment to make a copy (e.g., use list(error_msg) or error_msg.copy()) so
error_messages is a new list before extending with alternative_patterns; keep
the rest (alternative_patterns and extend) the same and only modify the creation
of error_messages to avoid mutating the passed-in error_msg.
| error_messages.extend(alternative_patterns) | ||
| libvirt.check_result(test_result, expected_fails=error_messages) | ||
| elif status_error: | ||
| libvirt.check_result(test_result, expected_fails=error_msg) |
There was a problem hiding this comment.
There is simipler fix. Check the exising codes, then you can find many similar fix. For example,
virtual_network/network_misc.cfg:44: expect_str = 'Expected integer value|Expected non-negative integer value'
There was a problem hiding this comment.
Like you mention i change it in cfg
… conflicts
Fix test failure caused by libvirt error message format change in newer
versions when validating boot order conflicts.
File: libvirt/tests/src/virtual_network/iface_attach_detach.py
Lines changed: 46-61 (added 15 lines)
The test validates that libvirt correctly rejects attaching a network
interface with boot order when os/boot is already configured on another
device. However, libvirt changed the error message format between versions,
causing the test to fail even though the error itself was correct.
Old libvirt error message:
"per-device boot elements cannot be used together with os/boot elements"
New libvirt error message:
"boot order 1 is already used by another device"
Problem:
The test expected only the old error message format from the error_msg
parameter, causing it to fail when libvirt returned the new message format.
The actual error was correct - libvirt properly rejected the invalid
configuration - but the message text differed.
Solution:
Modified error checking logic in test_boot_order() function to accept
both error message formats:
- If status_error is expected and error_msg is provided:
* Convert error_msg to list if it's a string
* Extend the list with known alternative patterns for boot order conflicts
* Pass the extended list to libvirt.check_result()
* This allows the test to pass if any pattern matches
- If status_error is expected but error_msg is empty:
* Call libvirt.check_result() with original parameters
The solution adds fallback patterns locally in the test rather than
modifying the shared libvirt.check_result() function in utils_test/libvirt.py,
ensuring this specific test works across libvirt versions without affecting
other tests that may depend on check_result() behavior.
Alternative patterns cover both old and new libvirt error message formats:
- "boot order .* is already used by another device"
- "per-device boot elements cannot be used together with os/boot elements"
Committer: Bolatbek Issakh <bissakh@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_attach_detach.py`:
- Around line 59-64: The code currently uses eval on user-supplied strings:
replace iface_attrs = eval(params.get("iface_attrs", "{}")) with a safe parser
using ast.literal_eval(params.get("iface_attrs", "{}")) and import ast at top;
for the dynamic dispatch replace test_func = eval(f"test_{case}") with an
explicit whitelist mapping like dispatch = {"boot_order": test_boot_order}
(validate case is in dispatch) and set test_func = dispatch[case] (raise a
controlled error if case is invalid) so only allowed functions are callable;
keep existing use of vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) and
virsh_options unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/iface_attach_detach.cfglibvirt/tests/src/virtual_network/iface_attach_detach.py
| iface_attrs = eval(params.get("iface_attrs", "{}")) | ||
| virsh_options = params.get("virsh_options", "") | ||
|
|
||
| bk_vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | ||
|
|
||
| # Get test function | ||
| test_func = eval('test_%s' % case) | ||
| test_func = eval(f"test_{case}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libvirt/tests/src/virtual_network/iface_attach_detach.py | head -80Repository: autotest/tp-libvirt
Length of output: 2489
🏁 Script executed:
grep -n "^def test_" libvirt/tests/src/virtual_network/iface_attach_detach.pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
grep -n "case" libvirt/tests/src/virtual_network/iface_attach_detach.py | head -20Repository: autotest/tp-libvirt
Length of output: 139
🏁 Script executed:
grep -n "def " libvirt/tests/src/virtual_network/iface_attach_detach.pyRepository: autotest/tp-libvirt
Length of output: 171
🏁 Script executed:
grep -n "case" libvirt/tests/src/virtual_network/iface_attach_detach.pyRepository: autotest/tp-libvirt
Length of output: 139
🏁 Script executed:
find libvirt/tests -name "*iface_attach*" -type f | head -20Repository: autotest/tp-libvirt
Length of output: 177
🏁 Script executed:
cat libvirt/tests/cfg/virtual_network/iface_attach_detach.cfgRepository: autotest/tp-libvirt
Length of output: 1378
Replace eval with safe parsing and explicit dispatch.
Lines 59 and 64 use eval(), creating unnecessary security and maintainability risks. Line 59 allows arbitrary code execution from params; use ast.literal_eval() instead to safely parse the dictionary. Line 64 uses eval() for dynamic function dispatch; replace with a whitelist map since only test_boot_order() is defined and the config specifies only "boot_order" as a valid case.
Proposed fix
+import ast
import logging
@@
- iface_attrs = eval(params.get("iface_attrs", "{}"))
+ iface_attrs = ast.literal_eval(params.get("iface_attrs", "{}"))
@@
- test_func = eval(f"test_{case}")
+ test_func_map = {
+ "boot_order": test_boot_order,
+ }
+ test_func = test_func_map.get(case)
+ if test_func is None:
+ test.error(f"Unsupported case: {case}")📝 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.
| iface_attrs = eval(params.get("iface_attrs", "{}")) | |
| virsh_options = params.get("virsh_options", "") | |
| bk_vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| # Get test function | |
| test_func = eval('test_%s' % case) | |
| test_func = eval(f"test_{case}") | |
| iface_attrs = ast.literal_eval(params.get("iface_attrs", "{}")) | |
| virsh_options = params.get("virsh_options", "") | |
| bk_vmxml = vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) | |
| test_func_map = { | |
| "boot_order": test_boot_order, | |
| } | |
| test_func = test_func_map.get(case) | |
| if test_func is None: | |
| test.error(f"Unsupported case: {case}") |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 59-59: Use of possibly insecure function; consider using ast.literal_eval
(S307)
[error] 64-64: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libvirt/tests/src/virtual_network/iface_attach_detach.py` around lines 59 -
64, The code currently uses eval on user-supplied strings: replace iface_attrs =
eval(params.get("iface_attrs", "{}")) with a safe parser using
ast.literal_eval(params.get("iface_attrs", "{}")) and import ast at top; for the
dynamic dispatch replace test_func = eval(f"test_{case}") with an explicit
whitelist mapping like dispatch = {"boot_order": test_boot_order} (validate case
is in dispatch) and set test_func = dispatch[case] (raise a controlled error if
case is invalid) so only allowed functions are callable; keep existing use of
vm_xml.VMXML.new_from_inactive_dumpxml(vm_name) and virsh_options unchanged.
| variants boot_order: | ||
| - -1: | ||
| error_msg = 'Expected .*integer value' | ||
| error_msg = 'Expected integer value|Expected non-negative integer value' |
There was a problem hiding this comment.
Does not old value match "Expected non-negative integer value"? Do you need this change indeed?
| error_msg = 'Expected integer value|Expected non-negative integer value' | ||
| - ss: | ||
| error_msg = 'Expected .*integer value' | ||
| error_msg = 'Expected integer value|Expected non-negative integer value' |
2 participants
Fix test failure caused by libvirt error message format change in newer versions when validating boot order conflicts.
File: libvirt/tests/src/virtual_network/iface_attach_detach.py
Lines changed: 46-61 (added 15 lines)
The test validates that libvirt correctly rejects attaching a network interface with boot order when os/boot is already configured on another device. However, libvirt changed the error message format between versions, causing the test to fail even though the error itself was correct.
Old libvirt error message:
"per-device boot elements cannot be used together with os/boot elements"
New libvirt error message:
"boot order 1 is already used by another device"
Problem:
The test expected only the old error message format from the error_msg parameter, causing it to fail when libvirt returned the new message format. The actual error was correct - libvirt properly rejected the invalid configuration - but the message text differed.
Solution:
Modified error checking logic in test_boot_order() function to accept both error message formats:
If status_error is expected and error_msg is provided:
If status_error is expected but error_msg is empty:
The solution adds fallback patterns locally in the test rather than modifying the shared libvirt.check_result() function in utils_test/libvirt.py, ensuring this specific test works across libvirt versions without affecting other tests that may depend on check_result() behavior.
Alternative patterns cover both old and new libvirt error message formats:
Committer: Bolatbek Issakh bissakh@redhat.com
Summary by CodeRabbit