nvme iopolicy get and set definitions in utils #6210
Conversation
Reference: avocado-framework#6183 Signed-off-by: Jan Richter <jarichte@redhat.com>
📝 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)
avocado/utils/nvme.py
Outdated
| cmd = f"cat /sys/class/nvme-subsystem/{subsystem}/iopolicy" | ||
| return process.run( | ||
| cmd, | ||
| shell=True, | ||
| sudo=True, | ||
| ignore_status=True | ||
| ).stdout_text.strip() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix command injection and path traversal risk in get_nvme_subsystem_io_policy()
Substituting subsystem directly into a shell command with shell=True and sudo=True is unsafe. A crafted subsystem (e.g., containing ; or $(...)) can inject commands, and ../ could lead to unintended paths.
Harden by validating/sanitizing the subsystem and quoting the path:
- 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()Add the missing import at the top of the file:
import shlex🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 541 to 547, the function builds a shell
command by interpolating the subsystem directly which allows command injection
and path traversal; validate the subsystem against an allowlist or a strict
pattern (e.g. only alphanumeric, hyphen, underscore), reject or raise on invalid
input, and avoid shell=True by constructing the path safely and passing it as an
argument to process.run (or use subprocess without shell) while quoting or
escaping the subsystem with shlex.quote if you must build a string; additionally
add the missing "import shlex" at the top of the file.
avocado/utils/nvme.py
Outdated
| cmd = f"echo {io_policy} > /sys/class/nvme-subsystem/{subsystem}/iopolicy" | ||
| if get_nvme_subsystem_io_policy(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}") | ||
| output = process.run( | ||
| f" cat /sys/class/nvme-subsystem/{subsystem}/iopolicy", | ||
| shell=True, | ||
| sudo=True, | ||
| ignore_status=True | ||
| ).stdout_text.strip() | ||
| return output == io_policy |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid shell injection and ensure sudo applies to redirection when changing iopolicy
- Command injection: both
subsystemandio_policyare interpolated into a shell withshell=True. This is exploitable. - Redirection under sudo:
echo ... > /sys/...can fail if the redirection is not performed with elevated privileges (depending on howsudois wrapped). Usesh -candteeto reliably write as root.
Harden and make the write reliable:
- 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}")
+ 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):
+ # Write as root: run the whole pipeline under the same sudo'ed shell
+ cmd = f"sh -c 'printf %s {shlex.quote(io_policy)} | tee {shlex.quote(path)} > /dev/null'"
+ if process.system(cmd, shell=True, sudo=True, ignore_status=True):
raise NvmeException(f"Changing nvme subsystem iopolicy is failed: {cmd}")
- output = process.run(
- f" cat /sys/class/nvme-subsystem/{subsystem}/iopolicy",
- shell=True,
- sudo=True,
- ignore_status=True
- ).stdout_text.strip()
+ output = process.run(
+ f"cat {shlex.quote(path)}",
+ shell=True,
+ sudo=True,
+ ignore_status=True,
+ ).stdout_text.strip()
return output == io_policyNote: This uses shlex.quote (add import shlex once at module top). Optionally, consider validating io_policy against allowed values (e.g., "round-robin", "numa") if you want stricter input validation.
To confirm how sudo is applied inside avocado.utils.process.run/system (i.e., whether it wraps sh -c), run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how sudo is implemented in process runner
rg -n -C5 'def run\(' avocado/utils/process.py || true
rg -n -C3 '\bsudo\b' avocado/utils/process.py || true
# Find existing patterns writing to /sys with sudo in the repo
rg -n -C2 -P 'echo\s+.+>\s*/sys/' || true
rg -n -C2 -P '\btee\s+/sys/' || trueLength of output: 10079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## _prepend_sudo definition (lines 620–680)"
sed -n '620,680p' avocado/utils/process.py
echo -e "\n## Top of avocado/utils/nvme.py (lines 1–50)"
head -n 50 avocado/utils/nvme.pyLength of output: 3459
Harden NVMe iopolicy write for injection safety and correct sudo redirection
The current echo … > path is executed by the outer shell (as non-root), so redirection will fail under sudo, and unquoted interpolation allows command injection. Update avocado/utils/nvme.py around lines 560–572 as follows:
• Validate and sanitize subsystem and (optionally) io_policy
• Build a quoted sysfs path
• Wrap the write in sh -c 'echo…>path' so the redirection runs under sudo
--- a/avocado/utils/nvme.py
+++ b/avocado/utils/nvme.py
@@ 557,6 +557,9 @@
:rtype: Boolean
"""
+ import re, shlex
+
# Sanitize subsystem name
- cmd = f"echo {io_policy} > /sys/class/nvme-subsystem/{subsystem}/iopolicy"
+ 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) validate io_policy against known values
+ # if io_policy not in ("round-robin", "numa"):
+ # raise NvmeException(f"Invalid I/O policy: {io_policy!r}")
+ path = f"/sys/class/nvme-subsystem/{safe_subsystem}/iopolicy"
if get_nvme_subsystem_io_policy(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}")
- output = process.run(
- f" cat /sys/class/nvme-subsystem/{subsystem}/iopolicy",
- shell=True,
- sudo=True,
- ignore_status=True
- ).stdout_text.strip()
- return output == io_policy
+ # Perform write under root so redirection is applied inside sudo’s shell
+ write_cmd = f"sh -c {shlex.quote(f'echo {io_policy} > {path}')}"
+ if process.system(write_cmd, shell=True, sudo=True, ignore_status=True):
+ raise NvmeException(f"Failed to change iopolicy: {write_cmd}")
+
+ # Verify the change
+ 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_policy- Adds
import re, shlexat the top of the function - Sanitizes
subsystemagainst path-traversal and injection - Quotes both
io_policyandpath - Wraps the echo/redirection in
sh -cto ensure the write happens as root
📝 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.
| cmd = f"echo {io_policy} > /sys/class/nvme-subsystem/{subsystem}/iopolicy" | |
| if get_nvme_subsystem_io_policy(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}") | |
| output = process.run( | |
| f" cat /sys/class/nvme-subsystem/{subsystem}/iopolicy", | |
| shell=True, | |
| sudo=True, | |
| ignore_status=True | |
| ).stdout_text.strip() | |
| return output == io_policy | |
| import re, shlex | |
| # Sanitize subsystem name | |
| 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) validate io_policy against known values | |
| # if io_policy not in ("round-robin", "numa"): | |
| # raise NvmeException(f"Invalid I/O policy: {io_policy!r}") | |
| path = f"/sys/class/nvme-subsystem/{safe_subsystem}/iopolicy" | |
| if get_nvme_subsystem_io_policy(subsystem) == io_policy: | |
| LOGGER.info("Returning True as iopolicy is same as current") | |
| return True | |
| # Perform write under root so redirection is applied inside sudo’s shell | |
| write_cmd = f"sh -c {shlex.quote(f'echo {io_policy} > {path}')}" | |
| if process.system(write_cmd, shell=True, sudo=True, ignore_status=True): | |
| raise NvmeException(f"Failed to change iopolicy: {write_cmd}") | |
| # Verify the change | |
| 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_policy |
🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 560 to 572, the current command uses
unquoted f-string interpolation and performs shell redirection outside sudo,
allowing command injection and failing to write as root; fix it by importing re
and shlex at the top of the function, validate/sanitize subsystem (reject
path-traversal and only allow safe characters, e.g. via a regex), optionally
validate io_policy, build a quoted sysfs path using shlex.quote, quote io_policy
with shlex.quote, and execute the write through sudo by running:
process.system(f"sh -c 'echo {quoted_io_policy} > {quoted_path}'", shell=True,
sudo=True, ignore_status=True) so the redirection runs under root; continue to
read back the file similarly quoted and compare the sanitized values.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6210 +/- ##
==========================================
+ Coverage 68.53% 73.12% +4.59%
==========================================
Files 205 206 +1
Lines 22412 22503 +91
==========================================
+ Hits 15359 16456 +1097
+ Misses 7053 6047 -1006 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
2 times, most recently
from
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)
|
Following is execution of the added definition using an nvme device
|
clebergnu
left a comment
clebergnu
left a comment
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.
avocado/utils/nvme.py
Outdated
| :param subsystem: Name of the subsystem | ||
| :rtype: String | ||
| """ | ||
| cmd = f"cat /sys/class/nvme-subsystem/{subsystem}/iopolicy" |
There was a problem hiding this comment.
Can you explain this runs cat instead of reading the file directly?
There was a problem hiding this comment.
@clebergnu Thanks for your input. Your input make sense. WIll change to file operation instead of reading directly
avocado/utils/nvme.py
Outdated
| :raises: NvmeException on command failures | ||
| :rtype: Boolean | ||
| """ | ||
| cmd = f"echo {io_policy} > /sys/class/nvme-subsystem/{subsystem}/iopolicy" |
There was a problem hiding this comment.
Same here... why running echo instead of writing to the file?
There was a problem hiding this comment.
@clebergnu Thanks for your input. Your input make sense. WIll change to file operation instead of writing directly
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
5d02f06 to
aed5b69
Compare
|
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.
avocado/utils/nvme.py
Outdated
| 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}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize subsystem and fix TOCTOU + file handle leak in reader.
- Path traversal: unvalidated
subsystemcan escape/sys/class/nvme-subsystem(e.g.,../../...). Avocado often runs as root; this is a real risk. - TOCTOU:
os.path.isfile()thenopen()is racy; handle viatry/except. - Leak:
open(...).readline()without a context manager leaves the fd open.
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")
- 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(f"Invalid subsystem name: {subsystem!r}")
+ 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(f"iopolicy file not found: {subsys_iopolicy_path}") from exc
+ except PermissionError as exc:
+ raise NvmeException("insufficient permissions to read iopolicy") from exc📝 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.
| 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(f"Invalid subsystem name: {subsystem!r}") | |
| 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(f"iopolicy file not found: {subsys_iopolicy_path}") from exc | |
| except PermissionError as exc: | |
| raise NvmeException("insufficient permissions to read iopolicy") from exc |
🧰 Tools
🪛 Ruff (0.12.2)
545-545: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 541 to 545, the code constructs a path
using an unvalidated subsystem, performs a TOCTOU check with os.path.isfile(),
and opens the file without a context manager; fix by validating/sanitizing the
subsystem (allow only a safe pattern such as /^[A-Za-z0-9._-]+$/), build the
path with os.path.join against the base "/sys/class/nvme-subsystem", resolve and
assert the realpath starts with that base to prevent path traversal, then remove
the separate isfile check and instead open the file inside a try/except using a
with-context to read the first line and strip the newline, catching
OSError/IOError and raising NvmeException with the error details on failure.
| 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}") | ||
|
|
||
|
|
||
| def change_nvme_subsystem_io_policy(subsystem, io_policy): | ||
| """ | ||
| Changes io policy of nvme device to specified io policy | ||
| Change in io policy is not persistent | ||
| :param subsystem: Name of the subsystem | ||
| :param io_policy: Name of the io_policy to which we want to set | ||
| :raises: NvmeException on command failures | ||
| :rtype: Boolean | ||
| """ | ||
| subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" | ||
| if get_nvme_subsystem_io_policy(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") 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}") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ruff TRY003: long exception messages outside exception class.
The linter flags Lines 545 and 567. Your refactor above shortens messages and uses exception chaining (from exc), which should satisfy TRY003. If the linter still complains, consider centralizing messages in NvmeException or suppressing locally with # noqa: TRY003.
Run Ruff to confirm the warnings are gone:
🏁 Script executed:
#!/bin/bash
ruff check avocado/utils/nvme.pyLength of output: 17396
Remove shell=True usage and centralize exception messages
- Replace all process.run and process.system calls that pass shell=True with safe argument lists (e.g.
["nvme", "id-ctrl", f"/dev/{controller_name}"]) to eliminate S604 findings. - Shorten or centralize all inline NvmeException messages (e.g. define standard error texts or codes in NvmeException) to satisfy TRY003; only use
# noqa: TRY003for any unavoidable one-off messages.
🧰 Tools
🪛 Ruff (0.12.2)
545-545: Avoid specifying long messages outside the exception class
(TRY003)
567-567: Avoid specifying long messages outside the exception class
(TRY003)
avocado/utils/nvme.py
Outdated
| subsys_iopolicy_path = f"/sys/class/nvme-subsystem/{subsystem}/iopolicy" | ||
| if get_nvme_subsystem_io_policy(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") 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) | ||
| return output == io_policy |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden writer: sanitize path, validate io_policy, drop redundant close, and handle permissions.
- Same path traversal concern as reader.
- Validate
io_policyto known values to fail fast. - Remove explicit
close()insidewith. - Surface
PermissionErrorasNvmeExceptionwith a clear hint; these writes usually require root.
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(f"Invalid subsystem name: {subsystem!r}")
+ if io_policy not in ALLOWED_NVME_IO_POLICIES:
+ raise NvmeException(f"Invalid io_policy: {io_policy!r}")
+ 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") 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(f"iopolicy file not found: {subsys_iopolicy_path}") 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 this constant near the top of the module (after LOGGER):
ALLOWED_NVME_IO_POLICIES = {"round-robin", "numa"}🧰 Tools
🪛 Ruff (0.12.2)
567-567: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 558 to 569, the writer is vulnerable to
path traversal, accepts arbitrary io_policy values, redundantly calls close()
inside a with block, and doesn't convert PermissionError into a clear
NvmeException; sanitize the subsystem path (e.g., use os.path.basename or
validate against an allowed pattern) before constructing subsys_iopolicy_path,
validate io_policy against ALLOWED_NVME_IO_POLICIES (add the constant as
requested), remove the explicit subsys_iopolicy_file.close() (rely on the with
context manager), wrap the open/write in a try/except that catches
PermissionError and raises NvmeException with a clear hint about needing root
permissions, and keep the existing existence check and final verification logic.
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
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)
avocado/utils/nvme.py
Outdated
| subsys_iopolicy_file.close() | ||
| else: | ||
| raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") | ||
| output = get_nvme_subsystem_io_policy(subsystem) |
There was a problem hiding this comment.
@maramsmurthy can we re-validate the iopolicy after change, like whether it changed successfully or not, then return it.
There was a problem hiding this comment.
Thanks for the input. Done with changes.
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
9fe2099 to
1302759
Compare
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. Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
maramsmurthy
force-pushed
the
smaram_nvme_iopolicy_change_new
branch
from
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.
| 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}") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Satisfy Ruff TRY003.
Shorten messages and/or raise from the underlying exception as shown above.
Run:
Also applies to: 566-566, 569-569
🏁 Script executed:
#!/bin/bash
ruff check avocado/utils/nvme.pyLength of output: 17897
Comply with Ruff TRY003 on exception messages
In avocado/utils/nvme.py at lines 544, 566 and 569 the literal exception texts exceed Ruff’s length limit—shorten them or chain from the original error (e.g. raise NvmeException("iopolicy file missing") from err).
🧰 Tools
🪛 Ruff (0.12.2)
544-544: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In avocado/utils/nvme.py around lines 544, 566 and 569 the raised NvmeException
messages are too long per Ruff TRY003; replace the long literal texts with
shorter messages and chain the original error when appropriate (e.g., use a
concise message like "iopolicy file missing" or "failed to read iopolicy" and
re-raise with from err) so the exception text is brief while preserving the
original exception context.
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
|
@clebergnu Can you please check and let us know if any changes are needed. If not can you please approve this request. |
richtja
left a comment
richtja
left a comment
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.
|
|
||
| name: Smokecheck on Linux with Python ${{ matrix.python-version }} | ||
| runs-on: ubuntu-22.04 | ||
|
|
There was a problem hiding this comment.
Can you please do a rebase and remove this commit from this PR? Thank you.
| subsys_iopolicy_file.close() | ||
| else: | ||
| raise NvmeException(f"iopolicy file not found: {subsys_iopolicy_path}") | ||
| if get_nvme_subsystem_io_policy(subsystem) == io_policy: |
There was a problem hiding this comment.
Can you please explain to me why the lines 567,568,569 are needed? IIUIC any issues with changing the policy will raise an error during the write operation at line 563 where you should handle it.
| 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() |
There was a problem hiding this comment.
Since you are using the open context manager, you don't need to call subsys_iopolicy_file.close(). It will be handled by the context manager.
4 participants
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