Added utilities definitions for nvme sed functionality - #6196
Conversation
📝 WalkthroughWalkthroughThis change adds comprehensive NVMe Self-Encrypting Drive (SED) locking management functions to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant nvme.py
participant pexpect
User->>nvme.py: is_lockdown_supported(namespace)
nvme.py->>nvme.py: Run 'nvme sed discover' and parse output
User->>nvme.py: is_lockdown_enabled(namespace)
nvme.py->>nvme.py: Run 'nvme sed discover' and parse output
User->>nvme.py: is_drive_locked(namespace)
nvme.py->>nvme.py: Run 'nvme sed discover' and parse output
User->>nvme.py: unlock_drive(namespace, with_pass_key)
nvme.py->>pexpect: Run 'nvme sed unlock' (send password if needed)
pexpect-->>nvme.py: Handle prompts and confirm unlock
User->>nvme.py: lock_drive(namespace, with_pass_key)
nvme.py->>pexpect: Run 'nvme sed lock' (send password if needed)
pexpect-->>nvme.py: Handle prompts and confirm lock
User->>nvme.py: revert_sed_locking(namespace, pwd)
nvme.py->>pexpect: Run 'nvme sed revert' with password
pexpect-->>nvme.py: Handle prompts and confirm revert
User->>nvme.py: change_sed_password(namespace, pwd1, pwd2)
nvme.py->>pexpect: Run 'nvme sed changepw' with old and new passwords
pexpect-->>nvme.py: Handle prompts and confirm password change
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)avocado/utils/disk.pyavocado/utils/nvme.pyNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Dear contributor, |
|
debug.log avocado run --max-parallel-tasks=1 nvmetest.py:NVMeTest.test_initialize_driver -m nvmetest.py.data/nvmetest.yamlFetching asset from nvmetest.py:NVMeTest.test_initialize_driver |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
562-564: Return the condition lockdown_attr["Locking Supported"] == "Yes" directly
Replace with return lockdown_attr["Locking Supported"] == "Yes"
(SIM103)
574-576: Return the condition lockdown_attr["Locking Feature Enabled"] == "Yes" directly
Replace with return lockdown_attr["Locking Feature Enabled"] == "Yes"
(SIM103)
586-588: Return the condition lockdown_attr["Locked"] == "Yes" directly
Replace with return lockdown_attr["Locked"] == "Yes"
(SIM103)
648-648: Local variable output is assigned to but never used
Remove assignment to unused variable output
(F841)
649-649: Undefined name log
(F821)
⏰ 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). (43)
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: Fedora selftests
- GitHub Check: Podman Egg task
- GitHub Check: Egg task debian:11.0
- GitHub Check: Egg task ubuntu:20.04
- GitHub Check: Egg task debian:12.4
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Egg task fedora:40
- GitHub Check: Version task ubi:8.8
- GitHub Check: Windows with Python 3.9
- GitHub Check: Windows with Python 3.10
- GitHub Check: Version task debian:12.4
- GitHub Check: Build Package (wheel/tarball) for Python 3.8
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (1)
avocado/utils/nvme.py (1)
28-28: LGTM!The
pexpectimport is correctly added to support interactive command execution for SED password operations.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6196 +/- ##
==========================================
- Coverage 68.14% 68.06% -0.09%
==========================================
Files 205 205
Lines 22411 22503 +92
==========================================
+ Hits 15273 15316 +43
- Misses 7138 7187 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d43fe64 to
c52270c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
535-556: Add command failure handling and input validation.The function needs to handle command execution failures and validate the namespace parameter as indicated in the docstring.
Apply this diff to implement proper error handling:
def get_nvme_sed_discover_parameters(namespace): """ Fetches values from nvme sed discover command :param namespace: NVMe namespace path :rtype: dictionary :raises: NvmeException on command failure - :rtype: dictionary """ + if not namespace: + raise NvmeException("Namespace parameter cannot be empty") + cmd = f"nvme sed discover {namespace}" - data = process.run(cmd, ignore_status=True, sudo=True, shell=True).stdout_text + result = process.run(cmd, ignore_status=True, sudo=True, shell=True) + if result.exit_status != 0: + raise NvmeException(f"Failed to discover SED parameters for {namespace}") + data = result.stdout_text pattern = r"\tLocking Supported:\s*(.*)\n\tLocking Feature Enabled:\s*(.*)\n\tLocked:\s*(.*)" match = re.search(pattern, data, re.MULTILINE) if match: locking_features = { "Locking Supported": match.group(1).strip(), "Locking Feature Enabled": match.group(2).strip(), "Locked": match.group(3).strip() } return locking_features - return {} + raise NvmeException(f"Unable to parse SED parameters for {namespace}")Also remove the duplicate
:rtype: dictionaryline at 542.
659-679: Fix undefined variable and incorrect logging.The function has an undefined variable
logand incorrectly logs a literal string instead of the output variable.Apply this diff to fix the logging issues:
for expect, value in list_of_expect_sendline: pexpect_handle.expect(expect, timeout=30) pexpect_handle.sendline(value) output = pexpect_handle.before.strip() - LOGGER.info("Pexpect output: %s", output) - log.info("output") + if output: + LOGGER.info("Pexpect output: %s", output) pexpect_handle.close()
🧹 Nitpick comments (3)
avocado/utils/nvme.py (3)
626-643: Fix docstring typo.The docstring parameter description contains a typo that needs to be fixed.
Apply this diff to fix the typo:
def lock_drive(namespace, with_pass_key=""): """ SED lock enables on nvme drive - :param namespace: NVMe namespace path (fix typo: namepsace -> namespace) + :param namespace: NVMe namespace path :param with_pass_key: Password for locking (if empty, no password prompt) """
645-657: Improve function documentation.The docstring needs parameter descriptions and proper formatting.
Apply this diff to improve the documentation:
def change_sed_password(namespace, pwd1, pwd2): """ - Changes older password of sed with provided password + Changes the SED password for the specified namespace + + :param namespace: NVMe namespace path + :param pwd1: Current SED password + :param pwd2: New SED password + :raises: NvmeException if password change is not supported or drive is not initialized """
594-679: Consider making timeout values configurable.Several functions use hardcoded timeout values (5 seconds for initialize_drive_locking, 30 seconds for pexpect_cmd_execution). Consider making these configurable for better flexibility.
Example approach:
# Add module-level constants PEXPECT_DEFAULT_TIMEOUT = 30 SED_COMMAND_TIMEOUT = 5 # Use in functions def pexpect_cmd_execution(cmd, list_of_expect_sendline, timeout=PEXPECT_DEFAULT_TIMEOUT): # ... pexpect_handle.expect(expect, timeout=timeout)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
674-674: Undefined name log
(F821)
⏰ 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). (28)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: Egg task fedora:41
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Fedora selftests
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Egg task fedora:40
- GitHub Check: Version task debian:11.0
- GitHub Check: Version task ubuntu:20.04
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task debian:12.4
- GitHub Check: Experimental checks
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Version task ubi:8.8
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Static checks
🔇 Additional comments (2)
avocado/utils/nvme.py (2)
558-592: LGTM!The boolean query functions are well-implemented with proper error handling using
.get()method and direct boolean returns.
607-624: LGTM!The unlock function is correctly implemented with proper pre-condition checks and post-operation verification.
3b15d2e to
d54aa7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
avocado/utils/nvme.py (1)
691-715: Fix critical typo causing runtime error.There's a typo on line 713 where
eishould bee, which will cause aNameErrorif an EOF exception occurs.except pexpect.exceptions.TIMEOUT as e: raise NvmeException(f"Command timeout: {cmd}") from e except pexpect.exceptions.EOF as e: - raise NvmeException(f"Command failed unexpectedly: {cmd}") from ei + raise NvmeException(f"Command failed unexpectedly: {cmd}") from e
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
536-557: Fix typo and improve error handling.
559-593: Simplify boolean returns and fix documentation issues.
🧹 Nitpick comments (2)
avocado/utils/nvme.py (2)
672-689: Improve parameter naming for clarity.The function logic is correct, but parameter names could be more descriptive.
-def change_sed_password(namespace, pwd1, pwd2): +def change_sed_password(namespace, current_password, new_password): """ Changes the SED password for the specified namespace :param namespace: NVMe namespace path - :param pwd1: Current SED password - :param pwd2: New SED password + :param current_password: Current SED password + :param new_password: New SED password :raises: NvmeException if password change is not supported or drive is not initialized """ if not is_lockdown_supported(namespace): raise NvmeException(f"Change password not supported on {namespace}") elif not is_lockdown_enabled(namespace): raise NvmeException(f"nvme drive {namespace} is not initialized, can't change password") pexpect_cmd_execution(f"nvme sed password {namespace}", - [("Password:", pwd1), - ("New Password:", pwd2), - ("Re-enter New Password:", pwd2)]) + [("Password:", current_password), + ("New Password:", new_password), + ("Re-enter New Password:", new_password)])
536-715: Consider adding unit tests for new SED functions.While the PR shows successful integration test execution, please ensure unit tests are added for all the new SED management functions to maintain code coverage and enable safe refactoring in the future.
Would you like me to help generate unit tests for these new SED functions or create a tracking issue for this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
712-712: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
713-713: Undefined name ei
(F821)
⏰ 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). (30)
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: Egg task fedora:41
- GitHub Check: Egg task debian:12.4
- GitHub Check: Fedora selftests
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task debian:12.4
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.8
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Version task ubi:8.8
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (4)
avocado/utils/nvme.py (4)
595-611: LGTM! Proper SED initialization with security considerations.The function correctly accepts password as a parameter (addressing past security concerns), validates preconditions, and verifies successful initialization.
613-632: Well-implemented SED revert function.The function properly validates all preconditions, uses descriptive parameter naming, and includes comprehensive documentation with error conditions.
634-651: Correct implementation of drive unlock functionality.The function properly handles both password-protected and passwordless unlock scenarios, with appropriate pre and post-condition checks.
653-670: Properly implemented drive lock functionality.The function correctly validates preconditions and verifies the drive is successfully locked after the operation.
6d2a940 to
5e24ac0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
avocado/utils/nvme.py (2)
536-543: Remove duplicate:rtype:entry in docstring.The docstring contains duplicate
:rtype: dictionaryentries.""" Fetches values from nvme sed discover command :param namespace: NVMe namespace path :rtype: dictionary :raises: NvmeException on command failure - :rtype: dictionary """
672-689: Consider adding verification after password change.The function should verify that the password change was successful, similar to other SED operations in this file.
You could add a verification step by attempting to perform an operation with the new password to ensure it was changed successfully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
711-711: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
712-712: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
713-713: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
714-714: 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). (35)
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Fedora selftests
- GitHub Check: macOS with Python 3.11
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
🔇 Additional comments (7)
avocado/utils/nvme.py (7)
559-569: LGTM!The function correctly uses
.get()method to safely handle missing dictionary keys.
571-581: LGTM!Consistent implementation with proper error handling.
583-593: LGTM!Well-implemented with consistent pattern and error handling.
595-611: LGTM!The function properly validates preconditions and verifies the operation success. Good that the password is now parameterized based on previous review feedback.
613-632: LGTM!Comprehensive validation logic with clear error messages for different failure scenarios.
634-651: LGTM!The function correctly handles both password-protected and passwordless unlock scenarios. Good that the logic errors identified in previous reviews have been addressed.
653-670: LGTM!The locking logic is correctly implemented with proper verification that the drive is locked after the operation.
ac5f95c to
032b836
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
avocado/utils/nvme.py (4)
536-556: Fix duplicate docstring entry and consider robustness improvements.The function has a minor documentation issue and could benefit from more robust parsing:
- Line 543: Duplicate
:rtype: dictionaryentry in docstring- The regex pattern assumes specific output formatting which could break if the command output format changes
Apply this diff to fix the docstring:
:param namespace: NVMe namespace path - :rtype: dictionary :raises: NvmeException on command failure :rtype: dictionaryConsider adding command success validation:
cmd = f"nvme sed discover {namespace}" data = process.run(cmd, ignore_status=True, sudo=True, shell=True).stdout_text + if not data or "error" in data.lower(): + raise NvmeException(f"Failed to discover SED parameters for {namespace}")
559-568: Fix inconsistent docstring description.The docstring still contains the old generic description "Fetches information based on namespace" on line 561, which conflicts with the more specific description on line 562.
Apply this diff to clean up the docstring:
""" - Fetches information based on namespace Checks if SED locking is supported for the given namespace
571-580: Fix inconsistent docstring description.The docstring has the same issue as the previous function - contains both old generic and new specific descriptions.
Apply this diff to clean up the docstring:
""" - Fetches information based on namespace Checks if SED locking feature is enabled for the given namespace
583-592: Fix inconsistent docstring description.The docstring contains the same inconsistency with both old and new descriptions.
Apply this diff to clean up the docstring:
""" - Fetches information based on namespace Checks if the drive is currently locked for the given namespace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(2 hunks)
⏰ 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). (41)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.12
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Build Package (wheel/tarball) for Python 3.8
- GitHub Check: Version task ubuntu:20.04
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task debian:11.0
- GitHub Check: Windows with Python 3.11
- GitHub Check: Windows with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (7)
avocado/utils/nvme.py (7)
29-31: LGTM! Imports are appropriate for the new SED functionality.The
sysandpexpectimports are correctly added to support interactive command execution and logging for the new NVMe SED management functions.
595-610: LGTM! Function properly handles SED initialization.The function correctly:
- Validates SED support and current state with appropriate precondition checks
- Uses parameterized password (addressing previous security concerns)
- Handles interactive password prompts via pexpect
- Validates successful initialization post-operation
613-631: LGTM! Comprehensive precondition checks and proper validation.The function demonstrates excellent defensive programming with:
- Support validation before attempting revert
- State validation (enabled but not locked) for safe operation
- Post-operation verification of successful revert
- Clear, specific error messages for each failure condition
634-650: LGTM! Critical logic issues from previous reviews have been resolved.The function now correctly:
- Uses proper conditional logic for password key detection
- Handles both password and non-password unlock scenarios appropriately
- Validates unlock success by checking if drive remains locked
- Provides clear error messages for different failure scenarios
653-669: LGTM! Logic error from previous review has been fixed.The function now properly:
- Validates pre-conditions (drive not already locked)
- Handles password and non-password scenarios correctly
- Uses correct post-operation validation to ensure locking succeeded
- Provides appropriate error messages
672-688: LGTM! Password change function is well-implemented.The function correctly:
- Validates SED support and initialization state
- Handles the complete password change workflow (current → new → confirm)
- Uses clear parameter naming convention
- Provides specific error messages for different failure conditions
691-714: LGTM! Helper function is robust with proper error handling.The function demonstrates excellent implementation with:
- Comprehensive logging of command execution and interactions
- Proper exception handling with chaining (
from e)- Clear timeout and EOF error differentiation
- Structured approach to expect/sendline interactions
- Good debugging support with before/after logging
The issues identified in previous reviews have been properly addressed.
032b836 to
33d235f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
avocado/utils/nvme.py (1)
691-711: Consider making sleep duration configurable.The hardcoded 3-second sleep after each
sendlinemight be excessive for some operations and insufficient for others. Consider making it configurable.-def pexpect_cmd_execution(cmd, list_of_expect_sendline): +def pexpect_cmd_execution(cmd, list_of_expect_sendline, sleep_after_sendline=3): """ Execute command using pexpect with multiple expect/sendline interactions :param cmd: Command to execute :param list_of_expect_sendline: List of (expect_pattern, sendline_value) tuples + :param sleep_after_sendline: Sleep duration in seconds after each sendline (default: 3) :raises: NvmeException on command failures """ try: LOGGER.info("Executing command using pexpect: %s", cmd) pexpect_handle = pexpect.spawn(cmd) for expect, value in list_of_expect_sendline: pexpect_handle.expect(expect, timeout=30) pexpect_handle.sendline(value) - time.sleep(3) + time.sleep(sleep_after_sendline) pexpect_handle.close() LOGGER.info("%s command executed successfully", cmd) except pexpect.exceptions.TIMEOUT as e: raise NvmeException(f"Command timeout: {cmd}") from e except pexpect.exceptions.EOF as e: raise NvmeException(f"Command failed unexpectedly: {cmd}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
29-29: sys imported but unused
Remove unused import: sys
(F401)
⏰ 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). (52)
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: Fedora selftests
- GitHub Check: Egg task debian:11.0
- GitHub Check: Egg task fedora:40
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Podman Egg task
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Windows with Python 3.12
- GitHub Check: Windows with Python 3.11
- GitHub Check: Windows with Python 3.13
- GitHub Check: Windows with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Version task ubi:8.8
- GitHub Check: Static checks
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (1)
avocado/utils/nvme.py (1)
672-689: LGTM! Well-structured password change function.The function properly validates prerequisites and handles the password change flow with confirmation.
d3fbe62 to
c167a01
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/disk.py(4 hunks)avocado/utils/nvme.py(12 hunks)spell.ignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spell.ignore
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/disk.py
121-123: Return the negated condition directly
Inline condition
(SIM103)
476-476: Use a context manager for opening files
(SIM115)
⏰ 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). (41)
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Fedora selftests
- GitHub Check: Egg task fedora:41
- GitHub Check: Egg task ubi:9.2
- GitHub Check: Version task ubuntu:20.04
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.8
- GitHub Check: macOS with Python 3.11
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Analyze (python)
🔇 Additional comments (15)
avocado/utils/disk.py (4)
77-78: LGTM!The explicit error checking with non-zero return code validation improves error handling robustness compared to the previous implementation.
81-82: LGTM!Consistent error handling pattern applied for loop device creation command.
105-106: LGTM!Consistent error handling pattern applied for loop device deletion command.
148-152: LGTM!The direct iteration over blockdevices and children eliminates intermediate list creation, improving performance and readability.
avocado/utils/nvme.py (11)
29-29: sys import is now used correctly.The sys import is now properly used in the pexpect_cmd_execution function (line 724:
pexpect_handle.log_read = sys.stdout), so the previous unused import concern has been resolved.
546-567: LGTM!The function implementation is solid with proper error handling, clear documentation, and robust regex parsing for the nvme sed discover command output.
570-579: LGTM!The function properly uses
.get()method for safe dictionary access and returns the boolean condition directly, addressing previous review feedback.
582-591: LGTM!Consistent implementation with proper error handling and direct boolean return.
594-603: LGTM!Consistent implementation pattern with the other lockdown status functions.
606-622: LGTM!The function properly addresses previous security concerns by accepting a password parameter instead of using hardcoded values. Error handling and validation logic are appropriate.
625-651: LGTM!The function includes comprehensive validation checks and supports both destructive and non-destructive revert operations with proper confirmation prompts.
654-671: LGTM!The logic errors from previous reviews have been properly fixed. The function correctly checks if the drive is locked after unlock attempts and uses proper conditional logic for password handling.
674-690: LGTM!The critical logic error has been fixed - the function now correctly checks if the drive is NOT locked after a lock operation, properly detecting failed lock attempts.
693-710: LGTM!Clean implementation with proper validation and clear parameter handling for SED password changes.
713-738: Excellent implementation with proper error handling.The function addresses all previous concerns:
- Proper logging using LOGGER instead of undefined 'log'
- Exception chaining with 'from e'
- Comprehensive error handling for pexpect exceptions
- Clear documentation
The sys.stdout usage for pexpect logging is appropriate and explains why the sys import is needed.
af834c1 to
6ed8aea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
avocado/utils/nvme.py (1)
723-723: Consider making pexpect stdout logging configurable.While
pexpect_handle.log_read = sys.stdoutis useful for debugging, it may be too verbose for production environments. Consider making this configurable or using a different logging approach.- pexpect_handle.log_read = sys.stdout + if LOGGER.isEnabledFor(logging.DEBUG): + pexpect_handle.log_read = sys.stdout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/disk.py(3 hunks)avocado/utils/nvme.py(3 hunks)spell.ignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spell.ignore
- avocado/utils/disk.py
⏰ 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). (40)
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Egg task debian:11.0
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Egg task ubuntu:20.04
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Version task fedora:41
- GitHub Check: Build Package (wheel/tarball) for Python 3.8
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
avocado/utils/nvme.py (9)
29-31: LGTM! Imports are properly utilized.Both
sysandpexpectimports are correctly used in the new SED functionality -sys.stdoutfor pexpect logging andpexpectfor interactive command handling.
537-558: LGTM! Well-structured SED parameter extraction.The function properly parses the nvme sed discover output using regex pattern matching and returns a structured dictionary. The error handling and documentation are appropriate.
561-594: LGTM! Clean and consistent boolean utility functions.All three functions follow a consistent pattern with proper boolean returns, safe dictionary access using
.get(), and clear documentation. The implementation is clean and efficient.
597-614: LGTM! Secure initialization with proper validation.The function correctly validates prerequisites (supported but not enabled), uses parameterized password instead of hardcoded values, and verifies successful initialization. The error handling is comprehensive.
617-647: LGTM! Comprehensive revert functionality with safety checks.The function properly handles both regular and destructive revert operations, with appropriate validation checks and confirmation prompts for destructive operations. The implementation is safe and well-documented.
650-666: LGTM! Correct unlock logic with proper validation.The function correctly validates that the drive is locked before attempting unlock, handles both password and non-password scenarios appropriately, and properly verifies successful unlock operation.
669-685: LGTM! Correct lock logic matching unlock pattern.The function properly validates preconditions, handles both password scenarios, and correctly verifies successful lock operation. The implementation is consistent with the unlock_drive function.
688-709: LGTM! Secure password change implementation.The function properly validates that SED is supported and enabled before attempting password change, and correctly handles the three-step password change process (current password, new password, confirm new password).
712-737: LGTM! Robust pexpect helper with comprehensive error handling.The function provides excellent interactive command handling with proper logging, exception chaining, and timeout management. The debug output and sleep intervals help with reliability.
78b774e to
44732fa
Compare
|
debug6.log |
48d195d to
74789f0
Compare
74789f0 to
ef3512a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
avocado/utils/nvme.py (1)
539-559: Enhance error handling and input validation.The function lacks proper error handling for command failures and input validation.
def get_nvme_sed_discover_parameters(namespace): """ Fetches values from nvme SED discover command :param namespace: NVMe namespace path - :rtype: dictionary :raises: NvmeException on command failure :rtype: dictionary """ + if not namespace: + raise NvmeException("Namespace parameter cannot be empty") + cmd = f"nvme sed discover {namespace}" - data = process.run(cmd, ignore_status=True, sudo=True, shell=True).stdout_text + result = process.run(cmd, ignore_status=True, sudo=True, shell=True) + if result.exit_status != 0: + raise NvmeException(f"Failed to discover SED parameters for {namespace}: {result.stderr_text}") + pattern = r"\tLocking Supported:\s*(.*)\n\tLocking Feature Enabled:\s*(.*)\n\tLocked:\s*(.*)" - match = re.search(pattern, data, re.MULTILINE) + match = re.search(pattern, result.stdout_text, re.MULTILINE) if match: locking_features = { "Locking Supported": match.group(1).strip(), "Locking Feature Enabled": match.group(2).strip(), "Locked": match.group(3).strip(), } return locking_features - return {} + raise NvmeException(f"Failed to parse SED discover output for {namespace}")Also fix the duplicate
:rtype:documentation on lines 545-546.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/disk.py(3 hunks)avocado/utils/nvme.py(4 hunks)spell.ignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spell.ignore
- avocado/utils/disk.py
⏰ 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). (320)
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
🔇 Additional comments (10)
avocado/utils/nvme.py (10)
18-18: LGTM: Author information updated.The author email has been properly updated in the file header.
132-132: LGTM: Substring index correction.The fix to use the correct substring index
len('nvme-subsys')instead of the previous incorrect index is appropriate.
562-571: LGTM: Function implementation looks correct.The function properly uses the discovery parameters and returns a boolean value directly. The docstring has been improved with better descriptions.
574-583: LGTM: Function implementation looks correct.The function properly checks if SED locking feature is enabled and returns the boolean result directly.
586-595: LGTM: Function implementation looks correct.The function properly checks if the drive is currently locked and returns the boolean result directly.
598-616: LGTM: Well-implemented SED initialization.The function properly validates prerequisites, uses configurable password parameters, and includes appropriate error handling. The logic flow and validation checks are sound.
619-648: LGTM: Comprehensive revert functionality.The function includes proper validation of prerequisites, supports both destructive and non-destructive revert modes, and has appropriate error handling. The destructive revert confirmation prompts are well-implemented for safety.
651-667: LGTM: Correct unlock implementation.The function properly validates that the drive is locked before attempting unlock, handles both password-based and non-password unlock scenarios, and correctly verifies the unlock operation succeeded.
670-686: LGTM: Correct lock implementation.The function properly validates that the drive is not already locked, handles both password-based and non-password lock scenarios, and correctly verifies the lock operation succeeded.
689-711: LGTM: Well-implemented password change functionality.The function includes proper validation to ensure SED is supported and enabled before attempting password change. The interactive prompts for current and new password (with confirmation) follow security best practices.
| import sys | ||
| import time | ||
|
|
||
| import pexpect # pylint: disable=E0401 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import and verify pexpect handling.
The sys import on line 30 appears to be used in the pexpect function (line 725), but this usage pattern may not be optimal for production code.
# Instead of redirecting to sys.stdout, consider using proper logging
pexpect_handle.log_read = sys.stdout # Current approach
# Better approach would be to capture output and log it properlyThe pexpect import with pylint disable is acceptable given the static analysis requirement mentioned in past comments.
🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 30 to 33, remove the unused sys import if
it is only used to redirect pexpect output to sys.stdout. Instead of assigning
pexpect_handle.log_read to sys.stdout, modify the code to capture the output
from pexpect and log it using the proper logging framework. Keep the pexpect
import with the pylint disable as is.
| def pexpect_cmd_execution(cmd, list_of_expect_sendline): | ||
| """ | ||
| Execute command using pexpect with multiple expect/sendline interactions | ||
|
|
||
| :param cmd: Command to execute | ||
| :param list_of_expect_sendline: List of (expect_pattern, sendline_value) tuples | ||
| :raises: NvmeException on command failures | ||
| """ | ||
| try: | ||
| LOGGER.info("Executing command using pexpect: %s", cmd) | ||
| pexpect_handle = pexpect.spawn(cmd) | ||
| pexpect_handle.log_read = sys.stdout | ||
| for expect, value in list_of_expect_sendline: | ||
| pexpect_handle.expect(expect, timeout=30) | ||
| pexpect_handle.sendline(value) | ||
| LOGGER.debug("Matched String: %s", pexpect_handle.after.strip()) | ||
| LOGGER.debug("Pexpect output: %s", pexpect_handle.before.strip()) | ||
| time.sleep(3) | ||
| pexpect_handle.close() | ||
| LOGGER.info("%s command executed successfully", cmd) | ||
| except pexpect.exceptions.TIMEOUT as e: | ||
| LOGGER.debug(e) | ||
| raise NvmeException(f"Command timeout: {cmd}") from e | ||
| except pexpect.exceptions.EOF as e: | ||
| LOGGER.debug(e) | ||
| raise NvmeException(f"Command failed unexpectedly: {cmd}") from e |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logging approach and consider security implications.
The function has several areas for improvement:
- Using
sys.stdoutfor pexpect logging may expose sensitive information - The sleep call is unnecessary and slows down execution
- Exception handling could be more specific
def pexpect_cmd_execution(cmd, list_of_expect_sendline):
"""
Execute command using pexpect with multiple expect/sendline interactions
:param cmd: Command to execute
:param list_of_expect_sendline: List of (expect_pattern, sendline_value) tuples
:raises: NvmeException on command failures
"""
try:
LOGGER.info("Executing command using pexpect: %s", cmd)
pexpect_handle = pexpect.spawn(cmd)
- pexpect_handle.log_read = sys.stdout
+ # Avoid logging to stdout to prevent exposing sensitive information
for expect, value in list_of_expect_sendline:
pexpect_handle.expect(expect, timeout=30)
pexpect_handle.sendline(value)
- LOGGER.debug("Matched String: %s", pexpect_handle.after.strip())
- LOGGER.debug("Pexpect output: %s", pexpect_handle.before.strip())
- time.sleep(3)
+ # Log without exposing sensitive values like passwords
+ LOGGER.debug("Matched expected pattern successfully")
pexpect_handle.close()
LOGGER.info("%s command executed successfully", cmd)
except pexpect.exceptions.TIMEOUT as e:
- LOGGER.debug(e)
+ LOGGER.error("Command timed out: %s", cmd)
raise NvmeException(f"Command timeout: {cmd}") from e
except pexpect.exceptions.EOF as e:
- LOGGER.debug(e)
+ LOGGER.error("Command ended unexpectedly: %s", cmd)
raise NvmeException(f"Command failed unexpectedly: {cmd}") from eThe current implementation may log sensitive information like passwords to stdout and debug logs.
📝 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.
| def pexpect_cmd_execution(cmd, list_of_expect_sendline): | |
| """ | |
| Execute command using pexpect with multiple expect/sendline interactions | |
| :param cmd: Command to execute | |
| :param list_of_expect_sendline: List of (expect_pattern, sendline_value) tuples | |
| :raises: NvmeException on command failures | |
| """ | |
| try: | |
| LOGGER.info("Executing command using pexpect: %s", cmd) | |
| pexpect_handle = pexpect.spawn(cmd) | |
| pexpect_handle.log_read = sys.stdout | |
| for expect, value in list_of_expect_sendline: | |
| pexpect_handle.expect(expect, timeout=30) | |
| pexpect_handle.sendline(value) | |
| LOGGER.debug("Matched String: %s", pexpect_handle.after.strip()) | |
| LOGGER.debug("Pexpect output: %s", pexpect_handle.before.strip()) | |
| time.sleep(3) | |
| pexpect_handle.close() | |
| LOGGER.info("%s command executed successfully", cmd) | |
| except pexpect.exceptions.TIMEOUT as e: | |
| LOGGER.debug(e) | |
| raise NvmeException(f"Command timeout: {cmd}") from e | |
| except pexpect.exceptions.EOF as e: | |
| LOGGER.debug(e) | |
| raise NvmeException(f"Command failed unexpectedly: {cmd}") from e | |
| def pexpect_cmd_execution(cmd, list_of_expect_sendline): | |
| """ | |
| Execute command using pexpect with multiple expect/sendline interactions | |
| :param cmd: Command to execute | |
| :param list_of_expect_sendline: List of (expect_pattern, sendline_value) tuples | |
| :raises: NvmeException on command failures | |
| """ | |
| try: | |
| LOGGER.info("Executing command using pexpect: %s", cmd) | |
| pexpect_handle = pexpect.spawn(cmd) | |
| # Avoid logging to stdout to prevent exposing sensitive information | |
| for expect, value in list_of_expect_sendline: | |
| pexpect_handle.expect(expect, timeout=30) | |
| pexpect_handle.sendline(value) | |
| # Log without exposing sensitive values like passwords | |
| LOGGER.debug("Matched expected pattern successfully") | |
| pexpect_handle.close() | |
| LOGGER.info("%s command executed successfully", cmd) | |
| except pexpect.exceptions.TIMEOUT as e: | |
| LOGGER.error("Command timed out: %s", cmd) | |
| raise NvmeException(f"Command timeout: {cmd}") from e | |
| except pexpect.exceptions.EOF as e: | |
| LOGGER.error("Command ended unexpectedly: %s", cmd) | |
| raise NvmeException(f"Command failed unexpectedly: {cmd}") from e |
🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 714 to 739, the function
pexpect_cmd_execution logs pexpect output directly to sys.stdout and debug logs,
which risks exposing sensitive data like passwords. To fix this, remove or
replace pexpect_handle.log_read = sys.stdout with a safer logging mechanism that
redacts sensitive info. Also, eliminate the unnecessary time.sleep(3) call to
improve performance. Finally, refine exception handling by catching more
specific exceptions if possible and ensure sensitive details are not logged in
exception messages.
Includes following 1. Initilaize SED on nvme disk. 2. Revert SED on nvme disk. 3. SED lock on nvme disk with and without key. 4. SED unlock on nvme disk with and without key. 5. Changes SED password. 6. Added pexpect definition to handle multiple inputs to same command. 7. Added dd definition to disk.py which return bool based of disk read. 8. Added new values to spell check file. Applied pep8 style cding to file Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
ef3512a to
c7b8ea2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
30-30: Address security concern with sys.stdout logging.While the
sysimport is used on line 725 forpexpect_handle.log_read = sys.stdout, this approach may expose sensitive information like passwords to stdout as identified in past reviews. Consider implementing a safer logging mechanism.
714-739: Security concern remains: stdout logging may expose sensitive data.While the function has been improved with proper exception handling and documentation, the
pexpect_handle.log_read = sys.stdouton line 725 still poses a security risk by potentially logging passwords and other sensitive information to stdout.
🧹 Nitpick comments (1)
avocado/utils/nvme.py (1)
539-559: Remove duplicate docstring entry.The function has a duplicate
:rtype: dictionaryentry on lines 545 and 546. Remove one of them for cleaner documentation.def get_nvme_sed_discover_parameters(namespace): """ Fetches values from nvme SED discover command :param namespace: NVMe namespace path - :rtype: dictionary :raises: NvmeException on command failure :rtype: dictionary """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
avocado/utils/disk.py(3 hunks)avocado/utils/nvme.py(4 hunks)spell.ignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spell.ignore
- avocado/utils/disk.py
⏰ 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). (20)
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Egg task ubuntu:20.04
- GitHub Check: Egg task fedora:40
- GitHub Check: Fedora selftests
- GitHub Check: Version task ubuntu:20.04
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Version task fedora:40
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Smokecheck on Linux with Python 3.8
- GitHub Check: Smokecheck on Linux with Python 3.12
- GitHub Check: Version task debian:12.4
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Version task ubi:8.8
- GitHub Check: Build Package (wheel/tarball) for Python 3.8
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
🔇 Additional comments (9)
avocado/utils/nvme.py (9)
132-132: LGTM! Typo fix in substring extraction.Good catch fixing the substring from 'nvme-subsy' to 'nvme-subsys' - this would have caused incorrect parsing of the subsystem name.
562-571: LGTM! Well-implemented boolean check.The function properly checks SED locking support with good error handling via the underlying
get_nvme_sed_discover_parametersfunction and clear documentation.
574-583: LGTM! Consistent implementation pattern.Good implementation following the same clean pattern as
is_lockdown_supportedwith appropriate error handling and documentation.
586-595: LGTM! Consistent boolean check implementation.Follows the established pattern for boolean checks with proper documentation and error handling.
598-616: LGTM! Security and validation improvements implemented.Good implementation with proper parameter validation, configurable password (addressing past security concerns), and comprehensive error checking.
619-648: LGTM! Comprehensive revert functionality.Excellent implementation with proper validation, support for both destructive and non-destructive revert operations, and clear documentation of the risks involved.
651-667: LGTM! Logic errors addressed.Good implementation with proper conditional handling for password-based and passwordless unlock operations, and correct validation of unlock success.
670-686: LGTM! Consistent with unlock functionality.Well-implemented lock functionality with proper state validation and consistent pattern matching the unlock_drive function.
689-711: LGTM! Secure password change implementation.Good implementation with proper validation checks and secure handling of password prompts through pexpect interactions.
PraveenPenguin
left a comment
PraveenPenguin
left a comment
There was a problem hiding this comment.
LGTM , will wait for CI to Pass
2d58369
into
avocado-framework:master
github-project-automation
Bot
moved this from Review Requested
to Done 112
in Default project
|
Hi @maramsmurthy and @PraveenPenguin, I have created a #6202 to revert this PR, because it introduces dependency on pexpect which is not in avocado requirements, and it will break avocado installation. It passed the CI because the check for import-errors has been disabled. Also, such change needs to be properly discussed in Avocado community. Please avoid such changes without proper discussion in the future. Thank you. |
4 participants
Includes following
Summary by CodeRabbit