Add function for secure capability verification #4232
Conversation
WalkthroughAdds two functions to virttest/utils_misc.py: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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. Comment |
bssrikanth
force-pushed
the
sev_check
branch
4 times, most recently
from
4d36838 to
7ecf2f5
Compare
|
@clebergnu @zixi-chen could you please review this and let me know your comments. TiA! |
|
@clebergnu @JinLiul @zixi-chen if you have any comments on this PR please let me know. Happy to address the review comments. |
bssrikanth
force-pushed
the
sev_check
branch
from
7ecf2f5 to
d2e4c70
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: 5
🧹 Nitpick comments (1)
virttest/utils_misc.py (1)
4742-4749: Use the existingcvm_typevariable for consistency.Line 4742 accesses
params["vm_secure_guest_type"]directly, butcvm_typewas already retrieved from the same key at line 4733. Use the existing variable for consistency and to avoid redundant dictionary lookups.Apply this diff:
expected_policy = vm.params.get_numeric("vm_sev_policy") - if params["vm_secure_guest_type"] == "snp": + if cvm_type == "snp": if "snp-policy" not in cvm_guest_info: raise exceptions.TestFail("QMP snp-policy not found in query-sev response.") actual_policy = cvm_guest_info["snp-policy"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/utils_misc.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/utils_misc.py (1)
virttest/qemu_monitor.py (4)
info(597-601)info(1067-1072)info(2367-2376)query_sev(3727-3735)
🪛 GitHub Actions: CI
virttest/utils_misc.py
[error] 1-1: isort: Imports are incorrectly sorted and/or formatted.
🪛 Ruff (0.14.8)
virttest/utils_misc.py
4714-4716: Avoid specifying long messages outside the exception class
(TRY003)
4718-4720: Avoid specifying long messages outside the exception class
(TRY003)
4722-4722: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4722-4722: Avoid specifying long messages outside the exception class
(TRY003)
4722-4722: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4737-4737: Abstract raise to an inner function
(TRY301)
4737-4737: Avoid specifying long messages outside the exception class
(TRY003)
4739-4739: Do not catch blind exception: Exception
(BLE001)
4740-4740: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4740-4740: Avoid specifying long messages outside the exception class
(TRY003)
4740-4740: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4744-4744: Avoid specifying long messages outside the exception class
(TRY003)
4748-4748: Avoid specifying long messages outside the exception class
(TRY003)
4751-4754: Avoid specifying long messages outside the exception class
(TRY003)
4757-4759: Avoid specifying long messages outside the exception class
(TRY003)
4765-4768: Abstract raise to an inner function
(TRY301)
4765-4768: Avoid specifying long messages outside the exception class
(TRY003)
4770-4770: Do not catch blind exception: Exception
(BLE001)
4771-4773: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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). (2)
- GitHub Check: fedora_40 AVOCADO_SRC:avocado-framework<104.0 SETUP:-m pip install . VT_TYPE:qemu
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install . VT_TYPE:qemu
🔇 Additional comments (1)
virttest/utils_misc.py (1)
4700-4723: Simplify logic and improve exception handling.The current implementation has redundant checks and could be streamlined:
- The
elseblock at lines 4717-4720 is unreachable dead code because ifos.path.exists(cvm_module_path)isFalse, the earlierifblock will never execute, making the logic confusing.IOErroris a legacy Python 2 exception; useOSErrorfor Python 3 compatibility.- Consider consolidating the existence check with file reading.
Apply this diff to simplify and improve the function:
def verify_sev_host(params): """ Verify if specific sev feature is enabled on the host :param params: Dictionary with the test parameters """ cvm_type = params["vm_secure_guest_type"] cvm_module_path = params["cvm_module_path"] LOG.info(f"Verifying cvm {cvm_type} capability enablement on host") + + if not os.path.exists(cvm_module_path): + raise exceptions.TestCancel( + f"Host support for {cvm_type} capability check failed: " + f"{cvm_module_path} does not exist." + ) + try: - if os.path.exists(cvm_module_path): - with open(cvm_module_path) as f: - output = f.read().strip() - if output not in params.objects("module_status"): - raise exceptions.TestCancel( - f"Host support for {cvm_type} capability check failed." - ) - else: + with open(cvm_module_path) as f: + output = f.read().strip() + if output not in params.objects("module_status"): + raise exceptions.TestCancel( + f"Host support for {cvm_type} capability check failed: " + f"module status '{output}' not in allowed statuses." + ) - raise exceptions.TestCancel( - f"Host support for {cvm_type} capability check failed." - ) - except IOError as e: - raise exceptions.TestCancel(f"Failed to read {cvm_module_path}: {str(e)}") + except OSError as e: + raise exceptions.TestCancel( + f"Failed to read {cvm_module_path}: {str(e)}" + ) from eLikely an incorrect or invalid review comment.
virttest/utils_misc.py
Outdated
| from avocado.utils import (aurl, distro, download, genio, git, linux_modules, | ||
| memory) |
There was a problem hiding this comment.
Fix import formatting to resolve CI failure.
The GitHub Actions CI is failing due to incorrectly sorted/formatted imports. Please run isort to fix the import order and formatting.
#!/bin/bash
# Check current import formatting
isort --check-only --diff virttest/utils_misc.pyAlso applies to: 53-54, 59-60, 65-70, 79-81
🤖 Prompt for AI Agents
In virttest/utils_misc.py around lines 46-47 (and also apply to ranges 53-54,
59-60, 65-70, 79-81), the imports are incorrectly sorted/formatted causing CI
isort failures; run isort on this file (or manually reorder imports to follow
project import grouping and alphabetical order) and reformat multi-line import
lists so each grouped import block conforms to isort/PEP8 expectations, then
re-run isort --check-only --diff to verify the fix and commit the updated file.
virttest/utils_misc.py
Outdated
| try: | ||
| cvm_guest_info = vm.monitor.query_sev() | ||
| if not cvm_guest_info: | ||
| raise exceptions.TestFail("QMP query-sev returned empty response.") | ||
| LOG.info(f"QMP cvm info: {cvm_guest_info}") | ||
| except Exception as e: | ||
| raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") |
There was a problem hiding this comment.
Avoid catching bare Exception.
Catching Exception at line 4739 is too broad and will catch system exceptions like KeyboardInterrupt and SystemExit. The QMP monitor commands typically raise more specific exceptions that should be caught explicitly. If the specific exception types are not documented, consider at least excluding system exceptions.
Consider this approach:
cvm_type = params["vm_secure_guest_type"]
try:
cvm_guest_info = vm.monitor.query_sev()
if not cvm_guest_info:
raise exceptions.TestFail("QMP query-sev returned empty response.")
LOG.info(f"QMP cvm info: {cvm_guest_info}")
- except Exception as e:
- raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}")
+ except exceptions.TestFail:
+ raise
+ except (KeyError, AttributeError, RuntimeError) as e:
+ raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") from e📝 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.
| try: | |
| cvm_guest_info = vm.monitor.query_sev() | |
| if not cvm_guest_info: | |
| raise exceptions.TestFail("QMP query-sev returned empty response.") | |
| LOG.info(f"QMP cvm info: {cvm_guest_info}") | |
| except Exception as e: | |
| raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") | |
| try: | |
| cvm_guest_info = vm.monitor.query_sev() | |
| if not cvm_guest_info: | |
| raise exceptions.TestFail("QMP query-sev returned empty response.") | |
| LOG.info(f"QMP cvm info: {cvm_guest_info}") | |
| except exceptions.TestFail: | |
| raise | |
| except (KeyError, AttributeError, RuntimeError) as e: | |
| raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") from e |
🧰 Tools
🪛 Ruff (0.14.8)
4737-4737: Abstract raise to an inner function
(TRY301)
4737-4737: Avoid specifying long messages outside the exception class
(TRY003)
4739-4739: Do not catch blind exception: Exception
(BLE001)
4740-4740: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4740-4740: Avoid specifying long messages outside the exception class
(TRY003)
4740-4740: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In virttest/utils_misc.py around lines 4734 to 4740, the except Exception: is
too broad and will swallow system-level exceptions; update the handler to only
catch the specific exceptions that vm.monitor.query_sev can raise (e.g.,
QMPError, MonitorError — replace with the actual monitor exception classes used
in the project) or, if those types are unknown, at minimum re-raise system
exceptions before converting others to TestFail. Change the block to catch the
specific monitor-related exception(s) or use a guard like: except Exception as
e: if isinstance(e, (KeyboardInterrupt, SystemExit)): raise; else: raise
exceptions.TestFail(f"QMP query-sev failed: {e}") so system interrupts propagate
while other errors are wrapped as TestFail.
virttest/utils_misc.py
Outdated
| guest_check_cmd = params["cvm_guest_check"] | ||
| try: | ||
| return_code, output = session.cmd_status_output(guest_check_cmd, timeout=240) | ||
| if return_code != 0: | ||
| raise exceptions.TestFail( | ||
| f"Guest cvm {cvm_type} capability check failed with " | ||
| "return code {return_code}: {output}" | ||
| ) | ||
| LOG.info(f"Guest cvm {cvm_type} capability check output: {output}") | ||
| except Exception as e: | ||
| raise exceptions.TestFail( | ||
| "Guest cvm {cvm_type} capability verify fail: %s" % str(e) | ||
| ) |
There was a problem hiding this comment.
Avoid catching bare Exception and improve exception chaining.
Line 4770 catches bare Exception, which is too broad. Additionally, the exception should be chained properly using raise ... from e to preserve the exception context.
Consider this approach:
LOG.info(f"Verifying cvm {cvm_type} capability enablement in guest")
guest_check_cmd = params["cvm_guest_check"]
try:
return_code, output = session.cmd_status_output(guest_check_cmd, timeout=240)
if return_code != 0:
raise exceptions.TestFail(
f"Guest cvm {cvm_type} capability check failed with "
f"return code {return_code}: {output}"
)
LOG.info(f"Guest cvm {cvm_type} capability check output: {output}")
- except Exception as e:
+ except exceptions.TestFail:
+ raise
+ except (aexpect.ShellError, aexpect.ShellTimeoutError) as e:
raise exceptions.TestFail(
- f"Guest cvm {cvm_type} capability verify fail: {str(e)}"
- )
+ f"Guest cvm {cvm_type} capability verify fail: {str(e)}"
+ ) from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
4765-4768: Abstract raise to an inner function
(TRY301)
4765-4768: Avoid specifying long messages outside the exception class
(TRY003)
4770-4770: Do not catch blind exception: Exception
(BLE001)
4771-4773: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In virttest/utils_misc.py around lines 4761 to 4773, avoid catching bare
Exception: restrict the except to only the expected errors from
session.cmd_status_output (e.g., TimeoutError, OSError, RuntimeError) or let
exceptions.TestFail propagate; when converting other errors to
exceptions.TestFail, re-raise using "raise exceptions.TestFail(...) from e" to
preserve context; also ensure your error messages use proper f-strings (replace
literal "{return_code}" and "{cvm_type}" inside non-f-strings) so the
logged/raised messages show actual values.
d2e4c70 to
65cb912
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
virttest/utils_misc.py (4)
4752-4755: Critical: Fix string formatting bug in error message.Line 4754 is missing the
fprefix, so{actual_policy}will be printed literally instead of showing the actual value. This bug was previously flagged but remains unaddressed.Apply this diff:
if actual_policy != expected_policy: raise exceptions.TestFail( f"QMP cvm policy mismatch: expected {expected_policy}, " - "got {actual_policy}" + f"got {actual_policy}" )
4766-4769: Critical: Fix string formatting bug in error message.Line 4768 is missing the
fprefix, so{return_code}and{output}will be printed literally. This bug was previously flagged but remains unaddressed.Apply this diff:
if return_code != 0: raise exceptions.TestFail( f"Guest cvm {cvm_type} capability check failed with " - "return code {return_code}: {output}" + f"return code {return_code}: {output}" )
4735-4741: Avoid catching bare Exception and add proper exception chaining.Catching bare
Exceptionat line 4740 is too broad and will catch system exceptions. Based on the relevant code snippets,vm.monitor.query_sev()can raise more specific exceptions. Additionally, line 4741 should use exception chaining.Consider this approach:
try: cvm_guest_info = vm.monitor.query_sev() if not cvm_guest_info: raise exceptions.TestFail("QMP query-sev returned empty response.") LOG.info(f"QMP cvm info: {cvm_guest_info}") - except Exception as e: - raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") + except exceptions.TestFail: + raise + except Exception as e: + raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") from e
4771-4774: Critical: Fix string formatting and improve exception handling.Line 4773 has two issues:
- Uses
%formatting with a string that contains{cvm_type}which won't be interpolated- Catches bare
Exceptionwhich is too broadThis bug was previously flagged but remains unaddressed.
Apply this diff:
except Exception as e: + # Re-raise TestFail without modification + if isinstance(e, exceptions.TestFail): + raise raise exceptions.TestFail( - "Guest cvm {cvm_type} capability verify fail: %s" % str(e) - ) + f"Guest cvm {cvm_type} capability verify fail: {str(e)}" + ) from e
🧹 Nitpick comments (1)
virttest/utils_misc.py (1)
4742-4742: Use the params parameter consistently.Line 4742 calls
vm.params.get_numeric("vm_sev_policy")but the function receivesparamsas a parameter. For consistency with the rest of the function (which usesparamsdirectly), consider using theparamsparameter here as well.Apply this diff if
paramsis the same object asvm.params:- expected_policy = vm.params.get_numeric("vm_sev_policy") + expected_policy = params.get_numeric("vm_sev_policy")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/utils_misc.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/utils_misc.py (3)
virttest/qemu_monitor.py (4)
info(597-601)info(1067-1072)info(2367-2376)query_sev(3727-3735)virttest/utils_params.py (1)
get_numeric(129-139)virttest/remote.py (1)
cmd_status_output(718-727)
🪛 GitHub Actions: CI
virttest/utils_misc.py
[error] 1-1: Black formatting would reformat this file. 1 file would be reformatted, 430 files would be left unchanged.
🪛 Ruff (0.14.8)
virttest/utils_misc.py
4716-4718: Avoid specifying long messages outside the exception class
(TRY003)
4720-4722: Avoid specifying long messages outside the exception class
(TRY003)
4724-4724: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4724-4724: Avoid specifying long messages outside the exception class
(TRY003)
4724-4724: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4738-4738: Abstract raise to an inner function
(TRY301)
4738-4738: Avoid specifying long messages outside the exception class
(TRY003)
4740-4740: Do not catch blind exception: Exception
(BLE001)
4741-4741: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4741-4741: Avoid specifying long messages outside the exception class
(TRY003)
4741-4741: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4745-4745: Avoid specifying long messages outside the exception class
(TRY003)
4749-4749: Avoid specifying long messages outside the exception class
(TRY003)
4752-4755: Avoid specifying long messages outside the exception class
(TRY003)
4758-4760: Avoid specifying long messages outside the exception class
(TRY003)
4766-4769: Abstract raise to an inner function
(TRY301)
4766-4769: Avoid specifying long messages outside the exception class
(TRY003)
4771-4771: Do not catch blind exception: Exception
(BLE001)
4772-4774: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
bssrikanth
force-pushed
the
sev_check
branch
4 times, most recently
from
13f5803 to
151d45a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
virttest/utils_misc.py (2)
4739-4745: Avoid catching bare Exception (duplicate concern).This was already flagged in previous reviews. Catching
Exceptionat line 4744 is too broad and will catch system exceptions likeKeyboardInterrupt. Based on the code context,vm.monitor.query_sev()likely raises QMP-specific exceptions.Consider restricting to specific exceptions:
try: cvm_guest_info = vm.monitor.query_sev() if not cvm_guest_info: raise exceptions.TestFail("QMP query-sev returned empty response.") LOG.info(f"QMP cvm info: {cvm_guest_info}") - except Exception as e: - raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") + except exceptions.TestFail: + raise + except (KeyError, AttributeError, RuntimeError) as e: + raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") from eTo verify the actual exception types raised by
query_sev(), run:#!/bin/bash # Search for exception types raised by query_sev and related QMP monitor methods ast-grep --pattern $'def query_sev($$$) { $$$ }' rg -nP -A10 "def query_sev" --type=py # Also check for QMP exception definitions rg -nP "class.*QMP.*Exception|class.*Monitor.*Exception" --type=py -g '!**/test/**'
4767-4778: Avoid catching bare Exception (duplicate concern).This was already flagged in previous reviews. Line 4775 catches bare
Exception, which is too broad. Thesession.cmd_status_output()method typically raisesaexpect.ShellErrororaexpect.ShellTimeoutError.Consider this approach:
try: return_code, output = session.cmd_status_output(guest_check_cmd, timeout=240) if return_code != 0: raise exceptions.TestFail( f"Guest cvm {cvm_type} capability check failed with " f"return code {return_code}: {output}" ) LOG.info(f"Guest cvm {cvm_type} capability check output: {output}") - except Exception as e: + except exceptions.TestFail: + raise + except (aexpect.ShellError, aexpect.ShellTimeoutError) as e: raise exceptions.TestFail( f"Guest cvm {cvm_type} capability verify fail: {str(e)}" - ) + ) from e
🧹 Nitpick comments (1)
virttest/utils_misc.py (1)
4703-4728: Consider adding a blank line before the function for Black formatting compliance.The code logic is sound. However, to comply with Black formatting standards, add a blank line before this function definition (after line 4701).
Apply this diff:
return False def verify_sev_host(params):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/utils_misc.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/utils_misc.py (3)
virttest/qemu_monitor.py (4)
info(597-601)info(1067-1072)info(2367-2376)query_sev(3727-3735)virttest/utils_params.py (1)
get_numeric(129-139)virttest/remote.py (1)
cmd_status_output(718-727)
🪛 Ruff (0.14.8)
virttest/utils_misc.py
4717-4719: Avoid specifying long messages outside the exception class
(TRY003)
4721-4723: Avoid specifying long messages outside the exception class
(TRY003)
4725-4727: Avoid specifying long messages outside the exception class
(TRY003)
4726-4726: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4742-4742: Abstract raise to an inner function
(TRY301)
4742-4742: Avoid specifying long messages outside the exception class
(TRY003)
4744-4744: Do not catch blind exception: Exception
(BLE001)
4745-4745: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4745-4745: Avoid specifying long messages outside the exception class
(TRY003)
4745-4745: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4749-4749: Avoid specifying long messages outside the exception class
(TRY003)
4753-4753: Avoid specifying long messages outside the exception class
(TRY003)
4756-4759: Avoid specifying long messages outside the exception class
(TRY003)
4762-4764: Avoid specifying long messages outside the exception class
(TRY003)
4770-4773: Abstract raise to an inner function
(TRY301)
4770-4773: Avoid specifying long messages outside the exception class
(TRY003)
4775-4775: Do not catch blind exception: Exception
(BLE001)
4776-4778: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4776-4778: Avoid specifying long messages outside the exception class
(TRY003)
4777-4777: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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). (1)
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install PYPI_UPLOAD/*.whl VT_TYPE:qemu
|
The testcase results which uses this api from avocado-vt: The PR for this testcase is at autotest/tp-qemu#4379 |
|
@PaulYuuu request your review. Thank you for your time in advance. |
virttest/utils_misc.py
Outdated
| cvm_module_path = params["cvm_module_path"] | ||
| LOG.info(f"Verifying cvm {cvm_type} capability enablement on host") | ||
| try: | ||
| if os.path.exists(cvm_module_path): | ||
| with open(cvm_module_path) as f: | ||
| output = f.read().strip() | ||
| if output not in params.objects("module_status"): |
There was a problem hiding this comment.
Some new parameters are introduced, but without a clear definition(e.g.: module_status), I would suggest using them in the function call instead of giving the raw params, to make this be a pure and clear function.
params --> module_name, module_status
BTW, if you only want to ensure the module with correct parameter loaded before test, maybe kvm_probe_module_parameters can help you, it will check kvm-amd for AMD, kvm-intel for Intel, and you can set it in cfg file like kvm_probe_module_parameters = "sev=1"
There was a problem hiding this comment.
The module_status parameter is used to check the status of secure guest type enablement on the host which is passed in via the [sev/snp/tdx]_module_path parameter.
If you recommend updating the definition for it, may I update the definition in virttest/shared/cfg/base.cfg under the # Secure guest params section?
Introduces a function to verify Secure (SEV/SEV-ES/SNP/TDX) capability enablement on the host as well as in a VM: 1. Validates host support for secure feature. 2. Queries SEV status via QMP and verifies policy. 3. Executes guest command to confirm secure guest enablement. Signed-off-by: Srikanth Aithal <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
virttest/utils_misc.py (2)
4772-4783: Critical: Replace bare Exception catch with specific exception types.Line 4780 catches bare
Exception, which is too broad and will swallow system exceptions. Thesession.cmd_status_output()method typically raisesaexpect.ShellErrororaexpect.ShellTimeoutErrorfor command execution issues. Catch these specifically instead.Additionally, lines 4781-4783 are missing exception chaining with
from e, and the f-string at line 4782 should use{e}directly instead of{str(e)}for better formatting.🔎 Recommended fix
try: return_code, output = session.cmd_status_output(guest_check_cmd, timeout=240) if return_code != 0: raise exceptions.TestFail( f"Guest cvm {secure_guest_type} capability check failed with " f"return code {return_code}: {output}" ) LOG.info(f"Guest cvm {secure_guest_type} capability check output: {output}") - except Exception as e: + except exceptions.TestFail: + raise + except (aexpect.ShellError, aexpect.ShellTimeoutError) as e: raise exceptions.TestFail( - f"Guest cvm {secure_guest_type} capability verify fail: {str(e)}" - ) + f"Guest cvm {secure_guest_type} capability verify fail: {e}" + ) from e
4742-4748: Critical: Replace bare Exception catch with specific exception types.Line 4747 catches bare
Exception, which will swallow system exceptions likeKeyboardInterruptandSystemExit. The QMP monitor'squery_sev()method should raise more specific exceptions that can be caught explicitly.Additionally, line 4748 is missing exception chaining with
from e.🔎 Recommended fix
try: cvm_guest_info = vm.monitor.query_sev() if not cvm_guest_info: raise exceptions.TestFail("QMP query-sev returned empty response.") LOG.info(f"QMP cvm info: {cvm_guest_info}") - except Exception as e: - raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") + except exceptions.TestFail: + raise + except (AttributeError, RuntimeError) as e: + raise exceptions.TestFail(f"QMP query-sev failed: {e}") from eBased on learnings, ...
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/utils_misc.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T08:39:59.775Z
Learnt from: zixi-chen
Repo: avocado-framework/avocado-vt PR: 4210
File: virttest/qemu_devices/qdevice_format.py:135-138
Timestamp: 2025-08-07T08:39:59.775Z
Learning: The policy parameter in the SEV-SNP guest object configuration can be provided as hexadecimal strings (e.g., "0x30000") which causes _str_to_dec to fail with ValueError. The _hex_in_str_to_dec function should be used instead to handle both decimal and hexadecimal string inputs correctly.
Applied to files:
virttest/utils_misc.py
🧬 Code graph analysis (1)
virttest/utils_misc.py (3)
virttest/qemu_monitor.py (4)
info(597-601)info(1067-1072)info(2367-2376)query_sev(3727-3735)virttest/utils_params.py (1)
get_numeric(129-139)virttest/remote.py (1)
cmd_status_output(718-727)
🪛 Ruff (0.14.10)
virttest/utils_misc.py
4718-4720: Avoid specifying long messages outside the exception class
(TRY003)
4722-4724: Avoid specifying long messages outside the exception class
(TRY003)
4726-4728: Avoid specifying long messages outside the exception class
(TRY003)
4727-4727: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4745-4745: Abstract raise to an inner function
(TRY301)
4745-4745: Avoid specifying long messages outside the exception class
(TRY003)
4747-4747: Do not catch blind exception: Exception
(BLE001)
4748-4748: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4748-4748: Avoid specifying long messages outside the exception class
(TRY003)
4748-4748: Use explicit conversion flag
Replace with conversion flag
(RUF010)
4752-4754: Avoid specifying long messages outside the exception class
(TRY003)
4758-4758: Avoid specifying long messages outside the exception class
(TRY003)
4761-4764: Avoid specifying long messages outside the exception class
(TRY003)
4767-4769: Avoid specifying long messages outside the exception class
(TRY003)
4775-4778: Abstract raise to an inner function
(TRY301)
4775-4778: Avoid specifying long messages outside the exception class
(TRY003)
4780-4780: Do not catch blind exception: Exception
(BLE001)
4781-4783: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
4781-4783: Avoid specifying long messages outside the exception class
(TRY003)
4782-4782: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (1)
virttest/utils_misc.py (1)
4703-4729: LGTM! Host verification function is well-structured.The function properly validates host support for secure guest features by:
- Dynamically constructing the module path based on guest type
- Checking file existence and readability
- Validating the module status against allowed values
- Using appropriate exception types with proper chaining
| LOG.info(f"QMP cvm info: {cvm_guest_info}") | ||
| except Exception as e: | ||
| raise exceptions.TestFail(f"QMP query-sev failed: {str(e)}") | ||
| expected_policy = vm.params.get_numeric("vm_sev_policy") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find where get_numeric is defined
rg "def get_numeric" --type pyRepository: avocado-framework/avocado-vt
Length of output: 156
🏁 Script executed:
# Get context around line 4749 in utils_misc.py
sed -n '4740,4775p' virttest/utils_misc.pyRepository: avocado-framework/avocado-vt
Length of output: 1792
🏁 Script executed:
# Find the Params class to understand get_numeric implementation
rg "class.*Params" virttest/ --type py | head -10Repository: avocado-framework/avocado-vt
Length of output: 290
🏁 Script executed:
# Get the full implementation of get_numeric() method
rg -A 10 "def get_numeric\(self, key, default=0, target_type=int\)" virttest/utils_params.pyRepository: avocado-framework/avocado-vt
Length of output: 535
🏁 Script executed:
# Check if there are any hex string handling utilities mentioned in the learnings context
rg "_hex_in_str_to_dec|_str_to_dec" virttest/ --type py | head -20Repository: avocado-framework/avocado-vt
Length of output: 1958
🏁 Script executed:
# Look for where vm_sev_policy parameter is used or defined
rg "vm_sev_policy" virttest/ --type pyRepository: avocado-framework/avocado-vt
Length of output: 360
🏁 Script executed:
# Check the implementation of _hex_in_str_to_dec to understand the proper approach
rg -B 5 -A 10 "def _hex_in_str_to_dec" virttest/qemu_devices/qdevice_format.pyRepository: avocado-framework/avocado-vt
Length of output: 450
🏁 Script executed:
# Verify if there are other places handling hex/decimal conversion for policy values
rg "policy" virttest/qemu_devices/qdevice_format.py -i -B 2 -A 2 | head -40Repository: avocado-framework/avocado-vt
Length of output: 214
🏁 Script executed:
# Get the full implementation of _hex_in_str_to_dec to see the complete conversion logic
rg -A 15 "def _hex_in_str_to_dec" virttest/qemu_devices/qdevice_format.pyRepository: avocado-framework/avocado-vt
Length of output: 447
🏁 Script executed:
# Check if there's a utility function for hex-decimal conversion available elsewhere
rg "def.*hex.*dec|def.*dec.*hex" virttest/ --type pyRepository: avocado-framework/avocado-vt
Length of output: 144
Use hex-aware conversion for SEV policy parameters.
The get_numeric() method calls Python's int() with a single argument, which cannot parse hexadecimal strings like "0x30000" and will raise a ValueError. Since policy parameters can be provided as hex strings (especially for SNP configurations), this needs to use a hex-aware conversion function similar to the _hex_in_str_to_dec pattern already established in the codebase for policy fields (see qdevice_format.py).
Apply this to line 4749 and also check qcontainer.py:4750 where int(params.get("vm_sev_policy", 3)) has the same issue.
🤖 Prompt for AI Agents
In virttest/utils_misc.py around line 4749 and qcontainer.py around line 4750,
the code uses get_numeric()/int() which fails on hex strings like "0x30000";
replace these conversions with the project's hex-aware conversion utility (e.g.,
call the existing _hex_in_str_to_dec or equivalent helper used for policy
fields) so that hex and decimal strings are both accepted, preserve the existing
default behavior when the param is missing or None, and ensure any returned
value is an int suitable for downstream use.
|
The testcase results which uses this api from avocado-vt: Run 1: with snp enabled on host kernel Run 2: with snp disabled on the host kernel PR for this testcase is at autotest/tp-qemu#4379 |
bssrikanth
changed the title
2 participants
Introduces a function to verify Secure Encrypted Virtualization (SEV) feature in a VM:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.