guest_os_booting/bootmenu: cover loadparm skip #6758
Conversation
On s390x, we can define an additional value `loadparm`. If given, the boot menu is skipped. Add test case for it. The absence of some string is harder to confirm reliably because we either would have to check the whole output with the disadvantage of slowing the test down, or risk checking on an invalid subset that is missing later messages. Solve this by 1. waiting a bit longer so that the first boot sequence messages after the boot menu will be included in the search 2. confirming the presence of two messages for test evaluation using them as limits between which the absence of the string that identifies the presence of the boot menu will be confirmed Signed-off-by: Sebastian Mitterle <[email protected]>
Apply black formatting rules. Signed-off-by: Sebastian Mitterle <[email protected]>
WalkthroughBoot menu config was extended to include the s390-virtio architecture and a new by_no_firmware variant. A loadparm_override variant was added under positive_test to set bootmenu_timeout and disk_order. The Python test was updated: added imports (re, libvirt_vmxml), introduced parameters check_not_prompt and disk_order, apply disk_order via libvirt_vmxml.modify_vm_device before boot, and implemented new serial-output validation that either waits for expected prompts or scans for absence of unwanted lines between two key points. Minor string/quotation normalizations were made. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
The previous commits would have a correctly failing test because the parameter wasn't set. Now set it, and also expect the value in the console during boot. Also, fix typo in (unused) parameter that is set empty explicitly in case there will be changes in handling it in the future. Signed-off-by: Sebastian Mitterle <[email protected]>
smitterl
force-pushed
the
cover_loadparm_skip
branch
from
5b20d11 to
f61bd93
Compare
![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
🤖 Fix all issues with AI agents
In @libvirt/tests/cfg/guest_os_booting/boot_menu/bootmenu.cfg:
- Line 49: Replace the misspelled configuration key `fireware_type` with the
correct `firmware_type` in the boot menu cfg entry so it matches other variants
and is recognized; locate the line containing `fireware_type = ""` in the
bootmenu.cfg (within the guest_os_booting/boot_menu section) and rename the key
to `firmware_type` preserving the value and formatting.
In @libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py:
- Around line 72-92: The current check_not_prompt block finds indices for the
two boundary patterns into matches/vals but never verifies their order, so if
check_not_prompt[2] occurs before check_not_prompt[0] the range check silently
skips; after computing vals = list(matches.values()) and before iterating
range(vals[0], vals[1]) add an explicit validation that vals[0] is less than
vals[1] (e.g., if vals[0] >= vals[1]: test.fail with a clear message referencing
check_not_prompt and the found indices), so the test fails when the boundary
patterns are out of expected order rather than silently passing.
- Around line 31-36: Replace unsafe eval usage by parsing literals with
ast.literal_eval for bootmenu_dict and disk_order (replace eval(params.get(...))
calls), and validate the parsed/derived check_not_prompt before indexing: after
splitting check_not_prompt, ensure it has at least three elements (or pad/raise
a clear error) so subsequent accesses to check_not_prompt[0], [1], [2] cannot
raise IndexError; update the code paths that use those indices to rely on the
validated/normalized list.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/guest_os_booting/boot_menu/bootmenu.cfglibvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/guest_os_booting/boot_menu/bootmenu.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
send(260-279)
🪛 Ruff (0.14.10)
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py
31-33: Use of possibly insecure function; consider using ast.literal_eval
(S307)
36-36: Use of possibly insecure function; consider using ast.literal_eval
(S307)
⏰ 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). (4)
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (6)
libvirt/tests/cfg/guest_os_booting/boot_menu/bootmenu.cfg (2)
6-7: LGTM - s390-virtio support added correctly.The addition of
s390-virtioalongsideaarch64properly extends boot menu support to s390x architecture as intended by this PR.
22-24: LGTM - loadparm_override variant configured correctly.The new test variant appropriately sets a non-zero boot menu timeout and specifies disk order with the
loadparmparameter, which aligns with the PR objective to test boot menu skip behavior when loadparm is configured.libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py (4)
10-10: LGTM - necessary imports added.The
remodule is used for pattern matching in the check_not_prompt logic, andlibvirt_vmxmlis required for disk device modification.Also applies to: 16-16
68-71: LGTM - prompt checking logic is sound.The addition of
check_promptvalidation usingread_until_any_line_matchesappropriately confirms the expected boot prompt appears within a reasonable timeout.
95-95: Quote style change is fine.The change from
'1'to"1"is a minor style consistency improvement with no functional impact.
49-50: No action needed. Themodify_vm_device()function handles VM configuration synchronization internally by default. The pattern used in this code (callingmodify_vm_device()without an explicitvmxml.sync()call) is consistent throughout the codebase and is the standard, correct usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py:
- Line 35: The code reads check_not_prompt = params.get("check_not_prompt",
"").split(",") but later assumes exactly three CSV entries (used at references
around lines handling patterns at usages on/near lines 76 and 88); add
validation immediately after that assignment to verify len(check_not_prompt) ==
3 and raise a clear ValueError (or a test-friendly exception) with a descriptive
message like "check_not_prompt must be three comma-separated patterns" so
misconfiguration fails early and avoids an IndexError at runtime.
🧹 Nitpick comments (2)
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py (2)
31-33: Consider usingast.literal_evalfor safer evaluation.Static analysis flags the use of
eval()as a security concern. While test configuration parameters are typically trusted,ast.literal_evalprovides a safer alternative that only evaluates Python literals (strings, numbers, tuples, lists, dicts, booleans, and None).🔒 Safer alternative using ast.literal_eval
Add the import at the top of the file:
import astThen replace the eval calls:
- bootmenu_dict = eval( - params.get("bootmenu_dict") % (bootmenu_enable, bootmenu_timeout) - ) + bootmenu_dict = ast.literal_eval( + params.get("bootmenu_dict") % (bootmenu_enable, bootmenu_timeout) + ) check_prompt = params.get("check_prompt") check_not_prompt = params.get("check_not_prompt", "").split(",") - disk_order = eval(params.get("disk_order", "{}")) + disk_order = ast.literal_eval(params.get("disk_order", "{}"))Also applies to: 36-36
72-92: Refine boundary handling and validation in boot menu absence check.The new logic to verify boot menu absence has a couple of edge cases to address:
Line 87:
range(vals[0], vals[1])includes the first boundary line itself. Typically, you'd want to exclude boundary lines from the search range:range(vals[0] + 1, vals[1]).No validation of boundary order: If the second boundary pattern appears before the first in the output, the current code will silently pass (empty range). Consider adding validation that
vals[0] < vals[1]to catch configuration errors.♻️ Enhanced boundary validation
vals = list(matches.values()) if None in vals: test.fail(f"Expected lines not found: {matches}") + if vals[0] >= vals[1]: + test.fail( + f"Boundary patterns appear in wrong order: " + f"first at line {vals[0]}, second at line {vals[1]}" + ) - for i in range(vals[0], vals[1]): + for i in range(vals[0] + 1, vals[1]): if re.match(check_not_prompt[1], output_lines[i]): test.fail( f"Found unexpected {check_not_prompt[1]}" f" at line {i} in output." )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/guest_os_booting/boot_menu/bootmenu.cfglibvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/guest_os_booting/boot_menu/bootmenu.cfg
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T08:01:27.899Z
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
Applied to files:
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py
🧬 Code graph analysis (1)
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py (1)
libvirt/tests/src/serial/serial_functional.py (1)
send(260-279)
🪛 Ruff (0.14.10)
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py
31-33: Use of possibly insecure function; consider using ast.literal_eval
(S307)
36-36: Use of possibly insecure function; consider using ast.literal_eval
(S307)
🔇 Additional comments (3)
libvirt/tests/src/guest_os_booting/boot_menu/bootmenu.py (3)
10-10: LGTM! Necessary imports for new functionality.Both imports are properly utilized:
refor regex pattern matching in the boot menu validation logiclibvirt_vmxmlfor disk device modification whendisk_orderis providedAlso applies to: 16-16
67-67: LGTM! Quote style normalization.Consistent use of double quotes for string literals aligns with the rest of the file.
Also applies to: 95-95
49-50: No action needed. The disk order changes vialibvirt_vmxml.modify_vm_device()are persisted automatically, as the function syncs the XML changes to libvirt internally by default.
1 participant
On s390x, we can define an additional value
loadparm. If given,the boot menu is skipped. Add test case for it.
The absence of some string is harder to confirm reliably because we
either would have to check the whole output with the disadvantage of
slowing the test down, or risk checking on an invalid subset that is
missing later messages.
Solve this by
the boot menu will be included in the search
as limits between which the absence of the string that identifies the
presence of the boot menu will be confirmed
Signed-off-by: Sebastian Mitterle [email protected]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.