nvme iopolicy get and set definitions in utils - #6210
Open
maramsmurthy wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds two utilities to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
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/nvme.pyTip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
avocado/utils/nvme.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
avocado/utils/nvme.py (1)
avocado/utils/process.py (2)
stdout_text(404-409)system(1023-1085)
⏰ 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). (48)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- 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-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-43-x86_64
- 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-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Version task debian:12.4
- GitHub Check: Egg task debian:11.0
- GitHub Check: Version task fedora:41
- GitHub Check: Egg task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Fedora selftests
- GitHub Check: Podman spawner with 3rd party runner plugin
- 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
- GitHub Check: Code Coverage (3.11)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6210 +/- ##
==========================================
- Coverage 71.69% 71.65% -0.05%
==========================================
Files 206 206
Lines 23480 23497 +17
==========================================
+ Hits 16834 16836 +2
- Misses 6646 6661 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
2 times, most recently
from
August 19, 2025 10:37
a7fea97 to
5d02f06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
534-545: Sanitize subsystem and avoid unquoted interpolation to prevent command injection/path traversalInterpolating
subsystemdirectly into a shell command withshell=Trueandsudo=Trueis unsafe. Validate and constrainsubsystem, build a safe path, and quote it.Apply this diff:
def get_nvme_subsystem_io_policy(subsystem): @@ - cmd = f"cat /sys/class/nvme-subsystem/{subsystem}/iopolicy" - return process.run( - cmd, shell=True, sudo=True, ignore_status=True - ).stdout_text.strip() + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException(f"Invalid subsystem name: {subsystem!r}") + path = f"/sys/class/nvme-subsystem/{safe_subsystem}/iopolicy" + cmd = f"cat {shlex.quote(path)}" + return process.run(cmd, shell=True, sudo=True, ignore_status=True).stdout_text.strip()Also add the missing import at the top of the file:
import shlex
547-567: Fix sudo redirection and injection risks when changing iopolicy; validate inputs and quote
- Redirection (
>) currently happens outside of sudo context; write can fail.- Both
subsystemandio_policyare unvalidated and interpolated into a shell.
Harden by validating inputs, quoting, and performing the write under a sudo'ed shell viatee.Apply this diff:
def change_nvme_subsystem_io_policy(subsystem, io_policy): @@ - cmd = f"echo {io_policy} > /sys/class/nvme-subsystem/{subsystem}/iopolicy" - if get_nvme_subsystem_io_policy(subsystem) == io_policy: + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException(f"Invalid subsystem name: {subsystem!r}") + # Optional: restrict to known iopolicy values if desired (e.g., "round-robin", "numa") + if not re.fullmatch(r"[A-Za-z0-9._-]+", io_policy): + raise NvmeException(f"Invalid iopolicy value: {io_policy!r}") + path = f"/sys/class/nvme-subsystem/{safe_subsystem}/iopolicy" + if get_nvme_subsystem_io_policy(safe_subsystem) == io_policy: LOGGER.info("Returning True as iopolicy is same as current") return True - if process.system(cmd, shell=True, sudo=True, ignore_status=True): - raise NvmeException(f"Changing nvme subsystem iopolicy is failed: {cmd}") - cmd = f"cat /sys/class/nvme-subsystem/{subsystem}/iopolicy" - output = process.run( - cmd, shell=True, sudo=True, ignore_status=True - ).stdout_text.strip() - return output == io_policy + # Run the redirection under sudo via a subshell; quote both value and path + write_cmd = f"sh -c 'printf %s {shlex.quote(io_policy)} | tee {shlex.quote(path)} > /dev/null'" + if process.system(write_cmd, shell=True, sudo=True, ignore_status=True): + raise NvmeException(f"Changing nvme subsystem iopolicy failed: {write_cmd}") + # Verify + read_cmd = f"cat {shlex.quote(path)}" + output = process.run(read_cmd, shell=True, sudo=True, ignore_status=True).stdout_text.strip() + return output == io_policyNote: Ensure
import shlexexists at module top (see previous comment).
🧹 Nitpick comments (1)
avocado/utils/nvme.py (1)
538-540: Docstring return types: prefer canonicalstr/boolMinor style nit: use
strandboolfor rtype consistency with Python typing.- :rtype: String + :rtype: str @@ - :rtype: Boolean + :rtype: boolAlso applies to: 555-556
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
avocado/utils/nvme.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
avocado/utils/nvme.py (1)
avocado/utils/process.py (2)
stdout_text(404-409)system(1023-1085)
⏰ 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). (53)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- 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-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- 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:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-43-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: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Egg task ubuntu:24.04
- GitHub Check: Version task ubuntu:24.04
- GitHub Check: Egg task debian:12.4
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Egg task fedora:41
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Fedora selftests
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Windows with Python 3.13
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
Contributor
Author
|
Following is execution of the added definition using an nvme device
|
clebergnu
requested changes
Sep 2, 2025
clebergnu
left a comment
clebergnu
left a comment
Contributor
There was a problem hiding this comment.
Hi @maramsmurthy ,
Unless I'm missing something, we should read and write to the /sys/* files directly instead of using cat and `echo.
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
September 9, 2025 06:52
5d02f06 to
aed5b69
Compare
Contributor
Author
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
545-545: Avoid specifying long messages outside the exception class
(TRY003)
567-567: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (29)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- 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-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Egg task ubuntu:24.04
- GitHub Check: Podman Egg task
- GitHub Check: Fedora selftests
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Egg task fedora:41
- GitHub Check: Build Egg for Python 3.11
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Analyze (python)
- GitHub Check: Windows with Python 3.11
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Version task ubi:8.8
- GitHub Check: Version task debian:12.4
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: macOS with Python 3.11
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Static checks
🔇 Additional comments (1)
avocado/utils/nvme.py (1)
534-569: Nice move: switched from shell to direct sysfs I/O.This addresses earlier injection and sudo-redirection pitfalls, and aligns with maintainers’ feedback.
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
September 9, 2025 08:53
aed5b69 to
a913484
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
534-545: Path traversal + TOCTOU + fd leak in reader; validate and use context managerUser-controlled
subsystemis interpolated into a filesystem path;isfile()+open()is racy; and theopen()handle isn’t closed. Harden and fix:- subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" - if os.path.isfile(subsys_iopolicy_path): - return open(subsys_iopolicy_path, "r", encoding="utf-8").readline().rstrip("\n") - else: - raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") + base_dir = "/sys/class/nvme-subsystem" + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException("invalid subsystem name") + subsys_iopolicy_path = os.path.join(base_dir, safe_subsystem, "iopolicy") + try: + with open(subsys_iopolicy_path, "r", encoding="utf-8") as f: + return f.readline().rstrip("\n") + except FileNotFoundError as exc: + raise NvmeException("iopolicy file not found") from exc + except PermissionError as exc: + raise NvmeException("insufficient permissions to read iopolicy") from exc
558-569: Harden writer: sanitize path, validate io_policy, handle permissions; drop redundant close()Same traversal risk exists; allow only known policies; rely on with-context (no explicit close); convert OSErrors into
NvmeException.- subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" - if get_nvme_subsystem_io_policy(subsystem) == io_policy: + base_dir = "/sys/class/nvme-subsystem" + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException("invalid subsystem name") + if io_policy not in ALLOWED_NVME_IO_POLICIES: + raise NvmeException("invalid io_policy") + subsys_iopolicy_path = os.path.join(base_dir, safe_subsystem, "iopolicy") + if get_nvme_subsystem_io_policy(safe_subsystem) == io_policy: LOGGER.info("Returning True as iopolicy is same as current") return True - if os.path.isfile(subsys_iopolicy_path): - with open(subsys_iopolicy_path, "w", encoding="utf-8") as subsys_iopolicy_file: - subsys_iopolicy_file.write(io_policy) - subsys_iopolicy_file.close() - else: - raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") - output = get_nvme_subsystem_io_policy(subsystem) + try: + with open(subsys_iopolicy_path, "w", encoding="utf-8") as f: + f.write(io_policy) + except FileNotFoundError as exc: + raise NvmeException("iopolicy file not found") from exc + except PermissionError as exc: + raise NvmeException("insufficient permissions to change iopolicy") from exc + output = get_nvme_subsystem_io_policy(safe_subsystem) return output == io_policyAdd the allowlist once near the top (after
LOGGER):ALLOWED_NVME_IO_POLICIES = {"round-robin", "numa"}
🧹 Nitpick comments (1)
avocado/utils/nvme.py (1)
545-545: Ruff TRY003: shorten exception messages or centralizeThese lines trigger TRY003. The diffs above shorten messages and use exception chaining; alternatively, centralize messages in
NvmeExceptionor add a local# noqa: TRY003.Also applies to: 567-567
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
545-545: Avoid specifying long messages outside the exception class
(TRY003)
567-567: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (23)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- 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-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Version task ubi:8.8
- GitHub Check: Build Package (wheel/tarball) for Python 3.12
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Version task fedora:40
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Fedora selftests
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Static checks
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
September 9, 2025 09:00
a913484 to
9fe2099
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
avocado/utils/nvme.py (1)
534-569: Address Ruff lint failures in avocado/utils/nvme.py
Ruff reports 29 violations—primarily:
- S604: insecure
shell=Truecalls inprocess.run/process.systemthroughout the file- TRY003: inline long exception messages instead of centralized definitions
• Replace all
shell=Trueinvocations with argument lists (e.g.["nvme", "id-ctrl", "/dev/…"])
• Refactor exception messages into NvmeException subclasses or constants rather than inline strings
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
541-545: Sanitize subsystem and fix TOCTOU + FD leak in reader (security + correctness).Unvalidated
subsystemenables path traversal;isfile()+open()is racy; directopen(...).readline()leaks the fd. Patch in-place:- subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" - if os.path.isfile(subsys_iopolicy_path): - return open(subsys_iopolicy_path, "r", encoding="utf-8").readline().rstrip("\n") - raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") + base_dir = "/sys/class/nvme-subsystem" + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException("invalid subsystem name") + subsys_iopolicy_path = os.path.join(base_dir, safe_subsystem, "iopolicy") + base_real = os.path.realpath(base_dir) + path_real = os.path.realpath(subsys_iopolicy_path) + if not path_real.startswith(base_real + os.sep): + raise NvmeException("invalid subsystem path") + try: + with open(subsys_iopolicy_path, "r", encoding="utf-8") as f: + return f.readline().rstrip("\n") + except FileNotFoundError as exc: + raise NvmeException("iopolicy file not found") from exc + except PermissionError as exc: + raise NvmeException("insufficient permissions to read iopolicy") from exc + except OSError as exc: + raise NvmeException("failed to read iopolicy") from exc
557-568: Harden writer: sanitize path, handle PermissionError/OSError, drop redundant close, avoid TOCTOU.Prevents traversal, converts OS errors to NvmeException, and removes
close()insidewith.- subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" - if get_nvme_subsystem_io_policy(subsystem) == io_policy: + base_dir = "/sys/class/nvme-subsystem" + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException("invalid subsystem name") + subsys_iopolicy_path = os.path.join(base_dir, safe_subsystem, "iopolicy") + base_real = os.path.realpath(base_dir) + path_real = os.path.realpath(subsys_iopolicy_path) + if not path_real.startswith(base_real + os.sep): + raise NvmeException("invalid subsystem path") + if get_nvme_subsystem_io_policy(safe_subsystem) == io_policy: LOGGER.info("Returning True as iopolicy is same as current") return True - if os.path.isfile(subsys_iopolicy_path): - with open(subsys_iopolicy_path, "w", encoding="utf-8") as subsys_iopolicy_file: - subsys_iopolicy_file.write(io_policy) - subsys_iopolicy_file.close() - else: - raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") - output = get_nvme_subsystem_io_policy(subsystem) + try: + with open(subsys_iopolicy_path, "w", encoding="utf-8") as f: + f.write(io_policy) + except FileNotFoundError as exc: + raise NvmeException("iopolicy file not found") from exc + except PermissionError as exc: + raise NvmeException("insufficient permissions to change iopolicy") from exc + except OSError as exc: + raise NvmeException("failed to change iopolicy") from exc + output = get_nvme_subsystem_io_policy(safe_subsystem) return output == io_policy
🧹 Nitpick comments (2)
avocado/utils/nvme.py (2)
547-556: Optional: validate io_policy to fail fast.Kernel typically accepts a small set (e.g., "round-robin", "numa"). Consider a local allowlist to catch typos before write.
Add near the top of the module (after LOGGER):
ALLOWED_NVME_IO_POLICIES = {"round-robin", "numa"}Inside this function (right after sanitizing subsystem):
+ if io_policy not in ALLOWED_NVME_IO_POLICIES: + raise NvmeException("invalid io_policy")
544-544: Lint: satisfy Ruff TRY003 on long exception messages.Shortening/standardizing messages as in the diffs above should clear TRY003 for these lines.
Also applies to: 566-566
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
544-544: Avoid specifying long messages outside the exception class
(TRY003)
566-566: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (21)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- 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-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: macOS with Python 3.11
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Windows with Python 3.13
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
Naresh-ibm
reviewed
Sep 9, 2025
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
2 times, most recently
from
September 10, 2025 05:33
1302759 to
37f33c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
avocado/utils/nvme.py (2)
541-544: Sanitize subsystem, avoid TOCTOU, and use a context manager (fix fd leak).
- Prevent path traversal with validation and safe joins.
- Replace
isfile()+open()withtry/except.- Read with
withto avoid leaking the descriptor.Apply:
- subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" - if os.path.isfile(subsys_iopolicy_path): - return open(subsys_iopolicy_path, "r", encoding="utf-8").readline().rstrip("\n") - raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") + base_dir = "/sys/class/nvme-subsystem" + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException("invalid subsystem") + subsys_iopolicy_path = os.path.join(base_dir, safe_subsystem, "iopolicy") + try: + with open(subsys_iopolicy_path, "r", encoding="utf-8") as f: + return f.readline().rstrip("\n") + except FileNotFoundError as exc: + raise NvmeException("iopolicy not found") from exc + except PermissionError as exc: + raise NvmeException("permission denied reading iopolicy") from exc
557-569: Harden writer: sanitize path, validate io_policy, remove redundant close, handle permissions, and re-read via sanitized name.
- Same traversal risk; validate
subsystem.- Validate
io_policyto known values.- Drop explicit
.close()insidewith.- Convert existence checks to
try/except.- Use
safe_subsystemon re-reads.Apply:
- subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" - if get_nvme_subsystem_io_policy(subsystem) == io_policy: + base_dir = "/sys/class/nvme-subsystem" + safe_subsystem = os.path.basename(subsystem) + if safe_subsystem != subsystem or not re.fullmatch(r"[A-Za-z0-9._-]+", safe_subsystem): + raise NvmeException("invalid subsystem") + if io_policy not in ALLOWED_NVME_IO_POLICIES: + raise NvmeException("invalid io_policy") + subsys_iopolicy_path = os.path.join(base_dir, safe_subsystem, "iopolicy") + if get_nvme_subsystem_io_policy(safe_subsystem) == io_policy: LOGGER.info("Returning True as iopolicy is same as current") return True - if os.path.isfile(subsys_iopolicy_path): - with open(subsys_iopolicy_path, "w", encoding="utf-8") as subsys_iopolicy_file: - subsys_iopolicy_file.write(io_policy) - subsys_iopolicy_file.close() - else: - raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") - if get_nvme_subsystem_io_policy(subsystem) == io_policy: + try: + with open(subsys_iopolicy_path, "w", encoding="utf-8") as f: + f.write(io_policy) + except FileNotFoundError as exc: + raise NvmeException("iopolicy not found") from exc + except PermissionError as exc: + raise NvmeException("permission denied changing iopolicy") from exc + if get_nvme_subsystem_io_policy(safe_subsystem) == io_policy: return True - raise NvmeException("Failed to change iopolicy") + raise NvmeException("failed to change iopolicy")Add near the module top (after LOGGER):
# module-level constant ALLOWED_NVME_IO_POLICIES = {"round-robin", "numa"}
🧹 Nitpick comments (4)
avocado/utils/nvme.py (4)
538-540: Docstring type nit.Use
strinstead ofString.- :rtype: String + :rtype: str
562-564: Redundant close() inside with.
with open(...):auto-closes the file.- with open(subsys_iopolicy_path, "w", encoding="utf-8") as subsys_iopolicy_file: - subsys_iopolicy_file.write(io_policy) - subsys_iopolicy_file.close() + with open(subsys_iopolicy_path, "w", encoding="utf-8") as f: + f.write(io_policy)
555-556: Docstring type nit.Use
boolinstead ofBoolean.- :rtype: Boolean + :rtype: bool
558-560: Log level suggestion.Consider downgrading to
DEBUGto reduce noise.- LOGGER.info("Returning True as iopolicy is same as current") + LOGGER.debug("iopolicy unchanged; returning True")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
avocado/utils/nvme.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
avocado/utils/nvme.py
544-544: Avoid specifying long messages outside the exception class
(TRY003)
566-566: Avoid specifying long messages outside the exception class
(TRY003)
569-569: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (21)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- 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-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: Version task fedora:41
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task fedora:40
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Fedora selftests
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Windows with Python 3.12
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Windows with Python 3.13
- 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)
534-540: Good shift to direct file I/O (no shell).Dropping
cat/echois safer and simpler. Nice.
Naresh-ibm
approved these changes
Sep 18, 2025
Contributor
There was a problem hiding this comment.
LGTM
@maramsmurthy Thanks Murthy for this PR
@clebergnu @PraveenPenguin can you please check this and merge, we need this for upcoming runs
Contributor
Author
|
@clebergnu Can you please check and let us know if any changes are needed. If not can you please approve this request. |
richtja
requested changes
Dec 2, 2025
richtja
left a comment
richtja
left a comment
Contributor
There was a problem hiding this comment.
Hi @maramsmurthy, sorry for the late reply. Your changes are almost ok, I have a just couple of comments to error handling and also request for removing the first two commits which are not related to this PR. Thank you.
pevogam
requested changes
Jun 19, 2026
pevogam
left a comment
pevogam
left a comment
Collaborator
There was a problem hiding this comment.
I think my wish here would be similar to the reply in multiple other similar commits - perhaps it could be moved directly to aautils in a two commit tier - one moving the file or at least for now creating a duplicate of the file (where we deprecate the current one) and one to introduce the actual diff by the original pull request.
maramsmurthy
added a commit
to maramsmurthy/avocado
that referenced
this pull request
Jun 23, 2026
1. get_nvme_subsystem_io_policy(subsystem): This function reads the current I/O policy for a specified NVMe subsystem from the /sys/class/nvme-subsystem/<subsystem>/iopolicy file and returns it as a string. It uses the process.run() function to execute the cat command with sudo privileges and ignores the command's status. 2. change_nvme_subsystem_io_policy(subsystem, io_policy): This function changes the I/O policy of an NVMe subsystem to the specified policy. It first checks if the current policy matches the desired policy and returns True if they are the same. If not, it executes the echo command to set the new policy. The function raises a NvmeException if the command fails and returns True if the policy was changed successfully. Both functions use the process.run() function from the process module to execute shell commands with sudo privileges and ignore their status. The get_nvme_subsystem_io_policy() function does not take any action if the current policy matches the desired policy, while the change_nvme_subsystem_io_policy() function raises an exception or returns True based on the outcome of the policy change. Review comment fixes: - Removed explicit close() call as context manager handles it automatically - Removed unnecessary verification after write operation as errors during write will be raised by the write operation itself Addresses review comments from PR avocado-framework#6210: - avocado-framework#6210 (comment) - avocado-framework#6210 (comment) Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
June 23, 2026 08:58
37f33c4 to
a298fed
Compare
maramsmurthy
added a commit
to maramsmurthy/avocado
that referenced
this pull request
Jun 23, 2026
1. get_nvme_subsystem_io_policy(subsystem): This function reads the current I/O policy for a specified NVMe subsystem from the /sys/class/nvme-subsystem/<subsystem>/iopolicy file and returns it as a string. It uses the process.run() function to execute the cat command with sudo privileges and ignores the command's status. 2. change_nvme_subsystem_io_policy(subsystem, io_policy): This function changes the I/O policy of an NVMe subsystem to the specified policy. It first checks if the current policy matches the desired policy and returns True if they are the same. If not, it executes the echo command to set the new policy. The function raises a NvmeException if the command fails and returns True if the policy was changed successfully. Both functions use the process.run() function from the process module to execute shell commands with sudo privileges and ignore their status. The get_nvme_subsystem_io_policy() function does not take any action if the current policy matches the desired policy, while the change_nvme_subsystem_io_policy() function raises an exception or returns True based on the outcome of the policy change. Review comment fixes: - Removed explicit close() call as context manager handles it automatically - Removed unnecessary verification after write operation as errors during write will be raised by the write operation itself Addresses review comments from PR avocado-framework#6210: - avocado-framework#6210 (comment) - avocado-framework#6210 (comment) Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
June 23, 2026 09:02
a298fed to
877c09a
Compare
maramsmurthy
added a commit
to maramsmurthy/avocado
that referenced
this pull request
Jun 23, 2026
1. get_nvme_subsystem_io_policy(subsystem): This function reads the current I/O policy for a specified NVMe subsystem from the /sys/class/nvme-subsystem/<subsystem>/iopolicy file and returns it as a string. It uses the process.run() function to execute the cat command with sudo privileges and ignores the command's status. 2. change_nvme_subsystem_io_policy(subsystem, io_policy): This function changes the I/O policy of an NVMe subsystem to the specified policy. It first checks if the current policy matches the desired policy and returns True if they are the same. If not, it executes the echo command to set the new policy. The function raises a NvmeException if the command fails and returns True if the policy was changed successfully. Both functions use the process.run() function from the process module to execute shell commands with sudo privileges and ignore their status. The get_nvme_subsystem_io_policy() function does not take any action if the current policy matches the desired policy, while the change_nvme_subsystem_io_policy() function raises an exception or returns True based on the outcome of the policy change. Review comment fixes: - Removed explicit close() call as context manager handles it automatically - Removed unnecessary verification after write operation as errors during write will be raised by the write operation itself Addresses review comments from PR avocado-framework#6210: - avocado-framework#6210 (comment) - avocado-framework#6210 (comment) Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
June 23, 2026 09:05
877c09a to
897a183
Compare
maramsmurthy
added a commit
to maramsmurthy/avocado
that referenced
this pull request
Jun 24, 2026
1. get_nvme_subsystem_io_policy(subsystem): This function reads the current I/O policy for a specified NVMe subsystem from the /sys/class/nvme-subsystem/<subsystem>/iopolicy file and returns it as a string. It uses the process.run() function to execute the cat command with sudo privileges and ignores the command's status. 2. change_nvme_subsystem_io_policy(subsystem, io_policy): This function changes the I/O policy of an NVMe subsystem to the specified policy. It first checks if the current policy matches the desired policy and returns True if they are the same. If not, it executes the echo command to set the new policy. The function raises a NvmeException if the command fails and returns True if the policy was changed successfully. Both functions use the process.run() function from the process module to execute shell commands with sudo privileges and ignore their status. The get_nvme_subsystem_io_policy() function does not take any action if the current policy matches the desired policy, while the change_nvme_subsystem_io_policy() function raises an exception or returns True based on the outcome of the policy change. Review comment fixes: - Removed explicit close() call as context manager handles it automatically - Removed unnecessary verification after write operation as errors during write will be raised by the write operation itself Code Coverage CI fix: - Fixed unreachable code in change_nvme_subsystem_io_policy() function - Reordered logic to check file existence BEFORE calling get_nvme_subsystem_io_policy() - This eliminates dead code path that was causing static analysis failures - The original code called get_nvme_subsystem_io_policy() which would raise an exception if file didn't exist, making the subsequent else block unreachable Addresses review comments from PR avocado-framework#6210: - avocado-framework#6210 (comment) - avocado-framework#6210 (comment) Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
June 24, 2026 05:18
897a183 to
72948a1
Compare
Contributor
Author
Thanks for the suggestion — that approach makes sense in general. For now, I’d prefer to keep the changes within the current scope of this PR. Since this is part of the initial discussion and aautils is still evolving, introducing a move/duplication layer at this stage might add some premature structure. |
maramsmurthy
added a commit
to maramsmurthy/avocado
that referenced
this pull request
Jun 24, 2026
1. get_nvme_subsystem_io_policy(subsystem): This function reads the current I/O policy for a specified NVMe subsystem from the /sys/class/nvme-subsystem/<subsystem>/iopolicy file and returns it as a string. It uses the process.run() function to execute the cat command with sudo privileges and ignores the command's status. 2. change_nvme_subsystem_io_policy(subsystem, io_policy): This function changes the I/O policy of an NVMe subsystem to the specified policy. It first checks if the current policy matches the desired policy and returns True if they are the same. If not, it executes the echo command to set the new policy. The function raises a NvmeException if the command fails and returns True if the policy was changed successfully. Both functions use the process.run() function from the process module to execute shell commands with sudo privileges and ignore their status. The get_nvme_subsystem_io_policy() function does not take any action if the current policy matches the desired policy, while the change_nvme_subsystem_io_policy() function raises an exception or returns True based on the outcome of the policy change. Review comment fixes: - Removed explicit close() call as context manager handles it automatically - Removed unnecessary verification after write operation as errors during write will be raised by the write operation itself Code Coverage CI fix: - Fixed unreachable code in change_nvme_subsystem_io_policy() function - Reordered logic to check file existence BEFORE calling get_nvme_subsystem_io_policy() - This eliminates dead code path that was causing static analysis failures - The original code called get_nvme_subsystem_io_policy() which would raise an exception if file didn't exist, making the subsequent else block unreachable Addresses review comments from PR avocado-framework#6210: - avocado-framework#6210 (comment) - avocado-framework#6210 (comment) Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
June 24, 2026 13:37
72948a1 to
a860a3f
Compare
Collaborator
|
Hmmm, I see. As similar rules and outcome to that of #6312 (comment) applies here, let's see if we reach any resolution there which would equally apply here. |
Contributor
Author
|
Addressed your review suggestions. Please approve it if you don't required any more changes. |
Add two utilities to avocado/utils/nvme.py for reading and writing
the iopolicy attribute of an NVMe subsystem via the sysfs interface:
get_nvme_subsystem_io_policy(subsystem)
Reads /sys/class/nvme-subsystem/<subsystem>/iopolicy using
genio.read_one_line() and returns the current policy as a str.
Raises NvmeException if the sysfs file is not found.
change_nvme_subsystem_io_policy(subsystem, io_policy)
Reads the current policy first; returns True immediately if it
already matches the requested value (no-op path). Otherwise
writes the new policy via genio.write_one_line().
Returns True on success; raises NvmeException if the sysfs
file is not found.
Both functions use direct sysfs file I/O via genio rather than
shelling out to cat/echo, and propagate FileNotFoundError as
NvmeException with exception chaining. The change is temporary
and does not persist across a reboot.
Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
June 29, 2026 09:08
a860a3f to
ce953ef
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added following definitions
get_nvme_subsystem_io_policy - > get iopolicy
change_nvme_subsystem_io_policy -> Set iopolicy temporarily. WIll not sustain after reboot of OS.
Summary by CodeRabbit