netkvm: add test for RX queue size parameter validation #4386
Conversation
WalkthroughAdds a Windows-only QEMU test configuration ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas to focus on:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Test Result: PASS |
![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)
qemu/tests/netkvm_rxcapacity.py (1)
27-44: Minor: Docstring parameter description could be more precise.Line 32 describes
vmas "QEMU test object", but it should be "VM object" since it's the virtual machine instance, not the test object (which is passed separately).Apply this diff if you'd like to improve the docstring:
- :param vm: QEMU test object + :param vm: VM object
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/netkvm_rxcapacity.cfg(1 hunks)qemu/tests/netkvm_rxcapacity.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/netkvm_rxcapacity.py (1)
qemu/tests/ovs_host_vlan.py (1)
ping(53-76)
🔇 Additional comments (5)
qemu/tests/cfg/netkvm_rxcapacity.cfg (1)
10-22: LGTM: Test variants are well-structured.The three test variants provide good coverage with different rx_queue_size and rx_capacity_value combinations.
qemu/tests/netkvm_rxcapacity.py (4)
1-5: LGTM: Imports are appropriate.All necessary modules are imported for the test functionality.
7-25: LGTM: Well-documented test entry point.The function signature and docstring clearly describe the test purpose and steps.
70-100: LGTM: Test setup and verification logic is sound.The test properly initializes the VM, sets RxCapacity, retrieves RxQueueSize from WMI, and verifies the values match expectations.
114-115: LGTM: Proper cleanup in finally block.The session is correctly closed in a finally block to ensure cleanup occurs even if the test fails.
heywji
force-pushed
the
add_netkvm_rxcapacity2
branch
from
69d06e3 to
e5a5d74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
qemu/tests/netkvm_rxcapacity.py (1)
27-45: Remove unusedsessionparameter in helper.
set_and_verify_rxcapacitynever references itssessionargument. Please drop it (and the corresponding argument at the call site) to avoid confusion about dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/netkvm_rxcapacity.cfg(1 hunks)qemu/tests/netkvm_rxcapacity.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/netkvm_rxcapacity.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/netkvm_rxcapacity.py (1)
qemu/tests/ovs_host_vlan.py (1)
ping(53-76)
qemu/tests/netkvm_rxcapacity.py
Outdated
| netkvm_wmi_path = params.get("wmi_cmd_path", "C:\\netkvm-wmi.cmd") | ||
| wmi_cmd_copy_cmd = params.get("wmi_cmd_copy_cmd") | ||
| wmi_cmd_copy_cmd = utils_misc.set_winutils_letter(session, wmi_cmd_copy_cmd) | ||
| session.cmd(wmi_cmd_copy_cmd, timeout=30) |
There was a problem hiding this comment.
Move this part to preparation phase would be better.
| vm.verify_alive() | ||
| session = vm.wait_for_serial_login(timeout=timeout) | ||
|
|
||
| virtio_win.prepare_netkvmco(vm) |
There was a problem hiding this comment.
You can copy netkvm-wmi.cmd after prepare_netkvmco
This patch adds a test to verify that QEMU's rx_queue_size parameter correctly influences the Windows guest's reported RxQueueSize value when RxCapacity is configured via netkvmco.exe. Signed-off-by: Wenkang Ji <[email protected]>
heywji
force-pushed
the
add_netkvm_rxcapacity2
branch
from
e5a5d74 to
f4ad792
Compare
|
@leidwang Hi Leidong, could you please review this patch again? Test Result: PASS |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
qemu/tests/netkvm_rxcapacity.py (2)
71-71: Verify parameter name matches config file.This line uses
params.get("netkvm_wmi_path", ...), but a previous review comment indicated this should be"wmi_cmd_path"to match the config file definition. The past comment was marked as "✅ Addressed in commit e5a5d74", but the code still shows"netkvm_wmi_path".Please verify:
- Is this a regression where the fix was reverted?
- Was the config file updated instead to use
"netkvm_wmi_path"?- Does the current configuration properly pass the WMI path?
Run the following script to check which parameter name is defined in the config file:
#!/bin/bash # Description: Check which parameter name is used in the config file # Search for wmi parameter definitions in the config file rg -n "wmi.*path" qemu/tests/cfg/netkvm_rxcapacity.cfg
103-114: Verify ping target is configurable.Lines 104-105 use a hardcoded external IP address (223.5.5.5) for the connectivity test. A previous review comment identified this as a major issue because it creates an external network dependency and will fail in isolated test environments. The past comment was marked as "✅ Addressed in commit e5a5d74", but the code still shows the hardcoded IP.
Please verify:
- Is this a regression where the fix was reverted?
- Is the connectivity test intended to always use this specific external IP?
- Should this be made configurable as originally suggested?
Run the following script to check if ping_target is defined in the config:
#!/bin/bash # Description: Check if ping_target parameter is defined in the config file # Search for ping_target parameter in the config file rg -n "ping_target" qemu/tests/cfg/netkvm_rxcapacity.cfg
🧹 Nitpick comments (2)
qemu/tests/netkvm_rxcapacity.py (2)
27-44: Remove unusedsessionparameter.The
sessionparameter is declared in the function signature and documented at line 31, but it's never used in the function body. The function only uses thevmparameter to set and get the netkvm parameter value.Consider removing the unused parameter to improve code clarity:
- def set_and_verify_rxcapacity(session, vm, test, value): + def set_and_verify_rxcapacity(vm, test, value): """ Set RxCapacity parameter and verify it was set correctly. - :param session: VM session info :param vm: QEMU test object :param test: QEMU test object :param value: Value to set for RxCapacityAnd update the call site at line 81:
- set_and_verify_rxcapacity(session, vm, test, rx_capacity_value) + set_and_verify_rxcapacity(vm, test, rx_capacity_value)
46-66: Consider moving WMI script copy to the preparation phase.Lines 52-53 copy the WMI script to the guest, which is a setup operation. This is currently embedded inside a helper function that's conceptually about retrieving data. For better separation of concerns and code clarity, consider moving the script copy operation to the main test flow, immediately after
prepare_netkvmcoat line 79.Based on learnings (leidwang's comment).
Suggested refactor:
def get_rxqueue_size_from_wmi(session, test, netkvm_wmi_path): """ Get RxQueueSize from netkvm-wmi.cmd output. :return: RxQueueSize value as integer """ - wmi_cmd_copy_cmd = utils_misc.set_winutils_letter(session, wmi_cmd_copy_cmd) - session.cmd(wmi_cmd_copy_cmd, timeout=30) - error_context.context("Getting RxQueueSize from WMI", test.log.info) wmi_output = session.cmd_output(f"{netkvm_wmi_path} cfg", timeout=30)And in the main test body:
virtio_win.prepare_netkvmco(vm) + +wmi_cmd_copy_cmd = utils_misc.set_winutils_letter(session, wmi_cmd_copy_cmd) +session.cmd(wmi_cmd_copy_cmd, timeout=30) + try: set_and_verify_rxcapacity(session, vm, test, rx_capacity_value) actual_rx_queue_size = get_rxqueue_size_from_wmi( - session, test, netkvm_wmi_path, wmi_cmd_copy_cmd + session, test, netkvm_wmi_path )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/netkvm_rxcapacity.cfg(1 hunks)qemu/tests/netkvm_rxcapacity.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/netkvm_rxcapacity.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/netkvm_rxcapacity.py (1)
qemu/tests/ovs_host_vlan.py (1)
ping(53-76)
2 participants
This patch adds a test to verify that QEMU's rx_queue_size parameter correctly influences the Windows guest's reported RxQueueSize value when RxCapacity is configured via netkvmco.exe.
ID: 2914
Signed-off-by: wji [email protected]
Summary by CodeRabbit