virtio_fs_rlimit_nofile: new case #4376
Conversation
WalkthroughAdds a Linux-focused VirtioFS RLIMIT_NOFILE test: new QEMU config and Python test that run virtiofsd with rlimit values 512, 610, 1000, and 2048, validating virtiofsd startup/log messages and guest-mounted filesystem behavior; includes Windows integration hooks and variant-specific expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Test Runner
participant Host as Host OS
participant virtiofsd as virtiofsd
participant Shell as ShellSession
participant VM as Guest VM
rect rgba(220,235,255,0.6)
note right of Runner: Variant rlimit_nofile = 512
Runner->>Host: run cmd_run_virtiofsd --rlimit-nofile 512 (process.run)
Host-->>Runner: non-zero exit + output
Runner->>Runner: assert expected_msg in output
end
rect rgba(230,255,230,0.6)
note right of Runner: Variant rlimit_nofile = 610
Runner->>Shell: start interactive aexpect session
Shell->>virtiofsd: launch --rlimit-nofile 610
virtiofsd-->>Shell: log stream
Shell-->>Runner: forwarded logs
Runner->>Runner: wait ≤10s for expected_msg
Runner->>Shell: close session
end
rect rgba(255,245,230,0.6)
note right of Runner: Variants rlimit_nofile = 1000 / 2048
Runner->>VM: verify VM alive & obtain login session
Runner->>Host: start virtiofsd with --rlimit-nofile N
Host-->>VM: expose shared-dir via socket
Runner->>VM: create/start guest service, mount fs, run list/check commands
VM-->>Runner: file listings / command status
Runner->>VM: stop/delete guest service and cleanup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
hellohellenmao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
qemu/tests/virtio_fs_rlimit_nofile.py (3)
2-4: Remove duplicate and unused imports
error_contextis imported twice;env_process,utils_disk, andutils_testare unused. Clean up to satisfy Ruff F811 and reduce noise.Apply:
-from virttest import error_context -from avocado.utils import process -from virttest import env_process, error_context, utils_disk, utils_misc, utils_test +from virttest import error_context +from avocado.utils import process +from virttest import utils_misc
43-61: Stabilize the interactive wait; optional tweaks
- Ten seconds can be tight on loaded hosts; consider 30s to cut flakes.
- Since you’re not interacting with a shell, the custom
promptisn’t needed and can mislead aexpect.Apply:
- session = aexpect.ShellSession( + session = aexpect.ShellSession( cmd_run_virtiofsd, auto_close=False, output_func=utils_misc.log_line, output_params=("virtiofs_fs-virtiofs.log",), - prompt=r"^\[.*\][\#\$]\s*$", ) @@ - session.expect(expected_msg, timeout=10) + session.expect(expected_msg, timeout=30)
71-71: Add trailing newline (W292)Keep linters quiet.
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
68-71: Consider regex for version-dependent warning textThe 610 message may vary slightly across virtiofsd versions (e.g., the remaining FD count). Using a regex reduces flakiness.
Example:
- expected_msg = "File descriptor count limit is very small, leaving only 0 file descriptors for the guest" + expected_msg = r"File descriptor count limit is very small, leaving only \d+ file descriptors for the guest"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
qemu/tests/virtio_fs_rlimit_nofile.py
4-4: Redefinition of unused error_context from line 2
Remove definition: error_context
(F811)
71-71: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (2)
1-1: Suite key syntax is correct The leading dash on- virtio_fs_rlimit_nofile:matches the established pattern in otherqemu/tests/cfgsuites.
64-66: Trailing space in rlimit value can break branch matchingThere’s a trailing space after 512; since the test compares strings, this could route into the “Invalid rlimit-nofile” failure.
Apply:
- rlimit_nofile = 512 + rlimit_nofile = 512Likely an incorrect or invalid review comment.
| no Host_RHEL.m6 Host_RHEL.m7 Host_RHEL.m8.u0 Host_RHEL.m8.u1 | ||
| type = virtio_fs_rlimit_nofile | ||
| required_virtiofsd_version = [1.13.2-1,) | ||
| virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Version check may under-report: include release in rpm query
cut -d- -f2 returns only VERSION (e.g., 1.13.2), but required_virtiofsd_version = [1.13.2-1,) expects VERSION-RELEASE. Use rpm queryformat.
Apply:
- virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2
+ virtiofsd_version_cmd = rpm -q --qf '%{VERSION}-%{RELEASE}\n' virtiofsd📝 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.
| virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2 | |
| virtiofsd_version_cmd = rpm -q --qf '%{VERSION}-%{RELEASE}\n' virtiofsd |
🤖 Prompt for AI Agents
In qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg around line 7, the rpm query uses
`cut -d- -f2` which returns only VERSION and omits RELEASE, causing
under-reporting against a required VERSION-RELEASE range; update the command to
use rpm's queryformat to emit both version and release (e.g. use rpm -q --qf
'%{VERSION}-%{RELEASE}\n' virtiofsd) so the produced value matches the
required_virtiofsd_version format.
| cmd_run_virtiofsd = params["cmd_run_virtiofsd"] | ||
|
|
There was a problem hiding this comment.
Avoid KeyError: defer fetching cmd_run_virtiofsd to the branches that need it
cmd_run_virtiofsd = params["cmd_run_virtiofsd"] is unconditional and will raise when running 1024/2048 variants that rely on fs harness flags instead of a direct command.
Move the lookup into the 512/610 branches:
- cmd_run_virtiofsd = params["cmd_run_virtiofsd"]
+ # Only required for 512/610 flows that invoke virtiofsd directlyAnd inside each branch:
@@
- result = process.run(
+ cmd_run_virtiofsd = params["cmd_run_virtiofsd"]
+ result = process.run(@@
- session = aexpect.ShellSession(
+ cmd_run_virtiofsd = params["cmd_run_virtiofsd"]
+ session = aexpect.ShellSession(📝 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_run_virtiofsd = params["cmd_run_virtiofsd"] | |
| @@ -21,7 +21,6 @@ def test_virtio_fs_rlimit_nofile(...): | |
| # Only required for 512/610 flows that invoke virtiofsd directly | |
| @@ -45,7 +45,8 @@ def test_virtio_fs_rlimit_nofile(...): | |
| cmd_run_virtiofsd = params["cmd_run_virtiofsd"] | |
| result = process.run( | |
| @@ -60,7 +61,8 @@ def test_virtio_fs_rlimit_nofile(...): | |
| cmd_run_virtiofsd = params["cmd_run_virtiofsd"] | |
| session = aexpect.ShellSession( |
🤖 Prompt for AI Agents
In qemu/tests/virtio_fs_rlimit_nofile.py around lines 21-22, remove the
unconditional lookup of cmd_run_virtiofsd from params (which raises KeyError for
the 1024/2048 variants) and instead fetch params["cmd_run_virtiofsd"] only
inside the branches that require it (the 512 and 610 branches); inside each of
those branches perform the lookup (or use params.get("cmd_run_virtiofsd") and
raise a clear error if missing) so the 1024/2048 paths that rely on fs harness
flags do not attempt to access the key.
1eea8fb to
d454af4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
qemu/tests/virtio_fs_rlimit_nofile.py (4)
48-69: Defer cmd lookup in 610 branch; ensure clean shutdown.Prevent KeyError by fetching the command here; also consider force-closing to avoid orphaned virtiofsd.
elif rlimit_nofile == "610": expected_msg = params["expected_msg"] error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info) + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' parameter for rlimit=610") session = aexpect.ShellSession( cmd_run_virtiofsd, auto_close=False, output_func=utils_misc.log_line, output_params=("virtiofs_fs-virtiofs.log",), prompt=r"^\[.*\][\#\$]\s*$", ) try: session.expect(expected_msg, timeout=10) test.log.info( "virtiofsd started successfully with the required message " "present" ) except aexpect.ExpectTimeout as e: test.fail("Timeout for virtiofsd start with rlimit-nofile=610: %s" % e) finally: - session.close() + # Prefer a hard close to avoid lingering daemon processes + session.close()
22-24: Avoid KeyError: fetch cmd_run_virtiofsd only in branches that need it.Accessing
params["cmd_run_virtiofsd"]unconditionally breaks 1024/2048 flows that rely on harness options instead of a direct command.rlimit_nofile = params.get("rlimit_nofile") -cmd_run_virtiofsd = params["cmd_run_virtiofsd"]
25-47: Also scan stderr; move cmd lookup into the 512 branch.virtiofsd often logs errors to stderr; current check only inspects stdout. Also defer
cmd_run_virtiofsdlookup here.if rlimit_nofile == "512": expected_msg = params["expected_msg"] error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info) + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' parameter for rlimit=512") result = process.run( cmd_run_virtiofsd, shell=True, ignore_status=True, verbose=True, ) - status, out = result.exit_status, result.stdout.decode() + status = result.exit_status + out_bytes = b"".join( + x for x in [ + (getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b""), + ] if x + ) + out = out_bytes.decode(errors="ignore") if status == 0: test.fail( "virtiofsd unexpectedly started successfully with " "rlimit-nofile=512" ) elif expected_msg not in out: test.fail( - "virtiofsd failed but without expected message. Output: %s" % out) + "virtiofsd failed but without expected message. Output: %s" % out) error_context.context( "virtiofsd failed as expected with the required message present", test.log.info)
70-78: Support rlimit-nofile=2048 to align with cfg variants.The cfg defines a 2048 case; current code fails it with “Invalid rlimit-nofile”.
- elif rlimit_nofile == "1024": + elif rlimit_nofile in ("1024", "2048"): error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info) vm = env.get_vm(params.get("main_vm")) vm.verify_alive() session = vm.wait_for_login() session.close()
🧹 Nitpick comments (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
70-77: Optionally validate mount availability inside the guest.If
fs_destis configured for 1024/2048 flows, do a quick accessibility check to catch mount issues early.vm = env.get_vm(params.get("main_vm")) vm.verify_alive() - session = vm.wait_for_login() - session.close() + session = vm.wait_for_login() + fs_dest = params.get("fs_dest") + if fs_dest: + rc = session.cmd_status("ls -ld %s" % fs_dest) + if rc != 0: + session.close() + test.fail("Guest mount point %s is not accessible" % fs_dest) + session.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🪛 Ruff (0.12.2)
qemu/tests/virtio_fs_rlimit_nofile.py
5-5: Redefinition of unused error_context from line 2
Remove definition: error_context
(F811)
hellohellenmao
force-pushed
the
4086
branch
from
d454af4 to
c05ec6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
qemu/tests/virtio_fs_rlimit_nofile.py (3)
2-6: Remove duplicate import of error_context (fix Ruff F811).
error_contextis imported twice (Lines 2 and 5).Apply:
-from virttest import error_context from avocado.utils import process from virttest import ( error_context, utils_disk, utils_misc, utils_test )
91-91: Avoid KeyError: fetch cmd_run_virtiofsd only in branches that need it.Unconditional lookup breaks 1000/2048 variants.
Apply:
@@ - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] @@ - result = process.run( + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + result = process.run( cmd_run_virtiofsd, shell=True, ignore_status=True, verbose=True, ) @@ - session = aexpect.ShellSession( + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + session = aexpect.ShellSession( cmd_run_virtiofsd, auto_close=False, output_func=utils_misc.log_line,Also applies to: 100-106, 123-125
106-114: Also scan stderr; many daemons log errors there.Currently only stdout is checked for the expected failure text.
Apply:
- status, out = result.exit_status, result.stdout.decode() + status = result.exit_status + out_bytes = b"".join( + x for x in [ + (getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b""), + ] if x + ) + out = out_bytes.decode(errors="ignore")
🧹 Nitpick comments (4)
qemu/tests/virtio_fs_rlimit_nofile.py (4)
59-61: Fix formatting of failure message.String isn’t an f-string;
{fs_target}is shown literally.Apply:
- test.fail("Failed to mount virtiofs {fs_target}.") + test.fail(f"Failed to mount virtiofs {fs_target}.")
156-157: Wrong variable in log; prints out instead of QEMU output.Should use
output.Apply:
- if params["qemu_check_content"] not in output: - test.fail(f"Wrong qemu content found: {out}") + if params["qemu_check_content"] not in output: + test.fail(f"Wrong qemu content found: {output}")
123-139: Ensure no orphaned virtiofsd after the 610-session check.Confirm
ShellSession.close()reliably terminates the spawned process; if not, send Ctrl-C or kill explicitly.Would you verify with
psafter the test that novirtiofsdremains? If needed, we can add a cleanup step to sendsession.sendcontrol('c')and wait for exit before closing.
178-178: Add trailing newline.Keeps linters happy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
🪛 Ruff (0.12.2)
qemu/tests/virtio_fs_rlimit_nofile.py
5-5: Redefinition of unused error_context from line 2
Remove definition: error_context
(F811)
hellohellenmao
force-pushed
the
4086
branch
from
c05ec6b to
16a3e31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
qemu/tests/virtio_fs_rlimit_nofile.py (6)
2-6: Remove duplicate error_context import (Ruff F811).Imported twice (Line 2 and Line 5). Keep one to satisfy linter.
from virttest import ( - error_context, utils_disk, utils_misc, utils_test + utils_disk, utils_misc, utils_test )
90-94: Defer cmd_run_virtiofsd lookup; avoid KeyError in 1000/2048 variants.Only 512/610 branches need it. Unconditional lookup can crash non‑virtiofsd paths.
rlimit_nofile = params.get("rlimit_nofile") -cmd_run_virtiofsd = params["cmd_run_virtiofsd"] guest_mnts = dict() os_type = params["os_type"]
95-106: 512: fetch cmd and read stderr too; many daemons log errors there.Move param fetch here and combine stdout+stderr for message check.
if rlimit_nofile == "512": expected_msg = params["expected_msg"] error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info) + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] result = process.run( cmd_run_virtiofsd, shell=True, ignore_status=True, verbose=True, ) - status, out = result.exit_status, result.stdout.decode() + status = result.exit_status + out_bytes = b"".join( + x for x in [ + (getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b""), + ] if x + ) + out = out_bytes.decode(errors="ignore")
118-129: 610: fetch cmd in-branch to prevent KeyError; keep behavior otherwise.Avoid global lookup; no functional change otherwise.
elif rlimit_nofile == "610": expected_msg = params["expected_msg"] error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info) + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] session = aexpect.ShellSession( cmd_run_virtiofsd, auto_close=False, output_func=utils_misc.log_line, output_params=("virtiofs_fs-virtiofs.log",), prompt=r"^\[.*\][\#\$]\s*$", )
171-174: Runtime bug: .strip() on tuple and printf token typo.cmd_status_output returns (status, output); also use “%s”, not “$s”.
- for fs_dest in guest_mnts.values(): - status, output = session.cmd_status_output(params["list_file_cmd"] % fs_dest).strip() - if status != 0: - test.fail("list failed: $s" % output) + for fs_dest in guest_mnts.values(): + status, output = session.cmd_status_output(params["list_file_cmd"] % fs_dest) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output)
140-177: Deduplicate 1000/2048 branches; reduce drift.Shared setup/teardown; diverge only on assertions.
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info) vm = env.get_vm(params.get("main_vm")) vm.verify_alive() session = vm.wait_for_login() try: session = create_service(session) session = start_service(session) for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - test.log.debug("The dir output in guest: %s", out) - if params["ls_check_content"] not in out: - test.fail(f"Wrong content found: {out}") - output = vm.process.get_output() - if params["qemu_check_content"] not in output: - test.fail(f"Wrong qemu content found: {out}") - finally: - stop_service(session) - delete_service() - elif rlimit_nofile == "2048": - error_context.context( - "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, - test.log.info) - vm = env.get_vm(params.get("main_vm")) - vm.verify_alive() - session = vm.wait_for_login() - try: - session = create_service(session) - session = start_service(session) - for fs_dest in guest_mnts.values(): - status, output = session.cmd_status_output(params["list_file_cmd"] % fs_dest) - output = output.strip() - if status != 0: - test.fail("list failed: %s" % output) + if rlimit_nofile == "1000": + out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() + test.log.debug("The dir output in guest: %s", out) + if params["ls_check_content"] not in out: + test.fail(f"Wrong content found: {out}") + qemu_out = vm.process.get_output() + if params["qemu_check_content"] not in qemu_out: + test.fail(f"Wrong qemu content found: {qemu_out}") + else: + status, output = session.cmd_status_output(params["list_file_cmd"] % fs_dest) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output) finally: stop_service(session) delete_service()
🧹 Nitpick comments (3)
qemu/tests/virtio_fs_rlimit_nofile.py (3)
155-157: Fix wrong variable in failure message.Should log QEMU output, not guest dir listing.
- output = vm.process.get_output() - if params["qemu_check_content"] not in output: - test.fail(f"Wrong qemu content found: {out}") + output = vm.process.get_output() + if params["qemu_check_content"] not in output: + test.fail(f"Wrong qemu content found: {output}")
100-139: Optional: Add a timeout to process.run and aexpect.expect for robustness.Prevents hangs if virtiofsd wedges; tune via params if needed.
- result = process.run( + result = process.run( cmd_run_virtiofsd, shell=True, ignore_status=True, - verbose=True, + verbose=True, + timeout=params.get_numeric("virtiofsd_timeout", 60), ) @@ - session.expect(expected_msg, timeout=10) + session.expect(expected_msg, timeout=params.get_numeric("virtiofsd_expect_timeout", 10))
178-178: Nit: Ensure file ends with a trailing newline.Minor POSIX/style nit; some tools warn without it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
🪛 Ruff (0.12.2)
qemu/tests/virtio_fs_rlimit_nofile.py
5-5: Redefinition of unused error_context from line 2
Remove definition: error_context
(F811)
hellohellenmao
force-pushed
the
4086
branch
from
16a3e31 to
3d1f539
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
qemu/tests/virtio_fs_rlimit_nofile.py (6)
57-60: Fix formatting bug in fail message (literal braces).String interpolation is missing; prints "{fs_target}" literally.
- test.fail("Failed to mount virtiofs {fs_target}.") + test.fail(f"Failed to mount virtiofs {fs_target}.")
1-3: LGTM: imports cleaned up; no duplicate error_context import in current revision.Previous duplication appears resolved.
89-89: Avoid KeyError: defer cmd_run_virtiofsd lookup to the branches that need it (512/610).Unconditional access will raise when running the 1000/2048 flows that don’t require this param.
Apply:
- cmd_run_virtiofsd = params["cmd_run_virtiofsd"]And fetch inside each branch:
@@ if rlimit_nofile == "512": + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' for rlimit_nofile=512") expected_msg = params["expected_msg"] @@ elif rlimit_nofile == "610": + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' for rlimit_nofile=610") expected_msg = params["expected_msg"]
104-111: Search stderr too; many daemons log errors there.Only checking stdout risks false negatives for the 512 failure path.
- status, out = result.exit_status, result.stdout.decode() + status = result.exit_status + out_bytes = b"".join( + x for x in [(getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b"")] if x + ) + out = out_bytes.decode(errors="ignore")
169-171: Runtime bug: calling .strip() on a tuple and printf token typo.cmd_status_output returns (status, output); .strip() on tuple raises; "$s" is invalid.
- status, output = session.cmd_status_output(cmd).strip() - if status != 0: - test.fail("list failed: $s" % output) + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output)
136-174: Deduplicate 1000/2048 branches to reduce drift.Logic is identical except validations; merge them for maintainability.
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): error_context.context( - "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info + "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info ) vm = env.get_vm(params.get("main_vm")) vm.verify_alive() session = vm.wait_for_login() try: session = create_service(session) session = start_service(session) for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - test.log.debug("The dir output in guest: %s", out) - if params["ls_check_content"] not in out: - test.fail(f"Wrong content found: {out}") - output = vm.process.get_output() - if params["qemu_check_content"] not in output: - test.fail(f"Wrong qemu content found: {out}") + cmd = params["list_file_cmd"] % fs_dest + if rlimit_nofile == "1000": + out = session.cmd_output(cmd).strip() + test.log.debug("The dir output in guest: %s", out) + if params["ls_check_content"] not in out: + test.fail(f"Wrong content found: {out}") + qemu_out = vm.process.get_output() + if params["qemu_check_content"] not in qemu_out: + test.fail(f"Wrong qemu content found: {qemu_out}") + else: + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output) finally: stop_service(session) delete_service() - elif rlimit_nofile == "2048": - ...
🧹 Nitpick comments (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
175-175: Add trailing newline at EOF.Minor style nit; helps some tools and avoids W292.
- +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (5)
create_viofs_service(318-363)delete_viofs_serivce(366-385)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
hellohellenmao
force-pushed
the
4086
branch
from
3d1f539 to
600bc14
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
qemu/tests/virtio_fs_rlimit_nofile.py (5)
57-59: Fix formatting bug in failure message (literal braces printed)The message prints "{fs_target}" literally.
Apply:
- test.fail("Failed to mount virtiofs {fs_target}.") + test.fail(f"Failed to mount virtiofs {fs_target}.")
136-174: Deduplicate 1000/2048 branches to reduce driftThe flows are near-identical. Merge and branch on rlimit_nofile inside the loop.
Example:
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): ... - for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - test.log.debug("The dir output in guest: %s", out) - if params["ls_check_content"] not in out: - test.fail("Wrong content found: %s" % out) - qemu_out = vm.process.get_output() - if params["qemu_check_content"] not in qemu_out: - test.fail("Wrong qemu content found: %s" % qemu_out) + for fs_dest in guest_mnts.values(): + cmd = params["list_file_cmd"] % fs_dest + if rlimit_nofile == "1000": + out = session.cmd_output(cmd).strip() + test.log.debug("The dir output in guest: %s", out) + if params["ls_check_content"] not in out: + test.fail("Wrong content found: %s" % out) + qemu_out = vm.process.get_output() + if params["qemu_check_content"] not in qemu_out: + test.fail("Wrong qemu content found: %s" % qemu_out) + else: + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output) - elif rlimit_nofile == "2048": - ...
88-92: Fix KeyError risk: only fetch cmd_run_virtiofsd in 512/610 branchesAccessing params["cmd_run_virtiofsd"] unconditionally breaks 1000/2048 flows that don’t need it.
Apply:
@@ - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] @@ if rlimit_nofile == "512": + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' when rlimit_nofile=512") @@ elif rlimit_nofile == "610": + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' when rlimit_nofile=610")
104-111: Search stderr too; daemons often emit errors thereCombine stdout+stderr before checking expected_msg and when reporting output.
Apply:
- status, out = result.exit_status, result.stdout.decode() + status = result.exit_status + out_bytes = b"".join( + x for x in [(getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b"")] if x + ) + out = out_bytes.decode(errors="ignore")
169-171: Runtime bug: calling .strip() on a tuple and printf token typocmd_status_output returns (status, output); .strip() on the tuple raises, and "$s" is invalid.
Apply:
- status, output = session.cmd_status_output(cmd).strip() - if status != 0: - test.fail("list failed: $s" % output) + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output)Run to catch similar patterns:
#!/bin/bash rg -nP --glob 'qemu/tests/**/*.py' 'cmd_status_output\([^)]*\)\.strip\(' -C2
🧹 Nitpick comments (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
120-136: Ensure virtiofsd is terminated in 610 flow to avoid orphansClose() may leave the daemon alive; try sending Ctrl-C first.
Apply:
finally: - session.close() + # Try to gracefully stop virtiofsd; then close the session. + try: + session.sendcontrol("c") + except Exception: + pass + session.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (5)
create_viofs_service(318-363)delete_viofs_serivce(366-385)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
hellohellenmao
force-pushed
the
4086
branch
from
600bc14 to
5a2e0e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
qemu/tests/virtio_fs_rlimit_nofile.py (3)
136-175: Deduplicate 1000/2048 branches to reduce driftBoth paths share setup/teardown; factor them into one branch and specialize the check.
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): @@ - for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - test.log.debug("The dir output in guest: %s", out) - if params["ls_check_content"] not in out: - test.fail("Wrong content found: %s" % out) - qemu_out = vm.process.get_output() - if params["qemu_check_content"] not in qemu_out: - test.fail("Wrong qemu content found: %s" % qemu_out) + for fs_dest in guest_mnts.values(): + cmd = params["list_file_cmd"] % fs_dest + if rlimit_nofile == "1000": + out = session.cmd_output(cmd).strip() + test.log.debug("The dir output in guest: %s", out) + if params["ls_check_content"] not in out: + test.fail("Wrong content found: %s" % out) + qemu_out = vm.process.get_output() + if params["qemu_check_content"] not in qemu_out: + test.fail("Wrong qemu content found: %s" % qemu_out) + else: + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output) - elif rlimit_nofile == "2048": - error_context.context( - "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info - ) - vm = env.get_vm(params.get("main_vm")) - vm.verify_alive() - session = vm.wait_for_login() - try: - session = create_service(session) - session = start_service(session) - for fs_dest in guest_mnts.values(): - cmd = params["list_file_cmd"] % fs_dest - status, output = session.cmd_status_output(cmd) - output = output.strip() - if status != 0: - test.fail("list failed: %s" % output) - finally: - stop_service(session) - delete_service()
88-91: Fix KeyError: fetch cmd_run_virtiofsd only in 512/610 branchesUnconditional lookup breaks 1000/2048 flows. Fetch lazily where needed.
@@ - cmd_run_virtiofsd = params["cmd_run_virtiofsd"]Apply inside rlimit 512:
@@ if rlimit_nofile == "512": expected_msg = params["expected_msg"] + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' for rlimit-nofile=512")And inside rlimit 610:
@@ elif rlimit_nofile == "610": expected_msg = params["expected_msg"] + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' for rlimit-nofile=610")
104-111: Also inspect stderr; many daemons log errors thereCurrent check misses messages printed to stderr.
- status, out = result.exit_status, result.stdout.decode() + status = result.exit_status + out_bytes = b"".join( + x for x in [(getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b"")] if x + ) + out = out_bytes.decode(errors="ignore") @@ - if status == 0: - test.fail( - "virtiofsd unexpectedly started successfully with rlimit-nofile=512" - ) + if status == 0: + test.fail( + "virtiofsd unexpectedly started successfully with rlimit-nofile=%s" + % rlimit_nofile + )
🧹 Nitpick comments (2)
qemu/tests/virtio_fs_rlimit_nofile.py (2)
128-133: Make expect timeout configurable10s can be tight on slower CI. Use a param with sane default.
- session.expect(expected_msg, timeout=10) + session.expect(expected_msg, timeout=int(params.get("expect_timeout", 30)))
22-35: Avoid relying on closed‑over vm; pass it explicitly to helpersPassing vm makes dependencies explicit and prevents surprises if the flow changes.
-def create_service(session): +def create_service(session, vm): @@ -def start_service(session): +def start_service(session, vm): @@ - if debug_log_operation: - session = virtio_fs_utils.operate_debug_log( - test, params, session, vm, debug_log_operation - ) + if debug_log_operation: + session = virtio_fs_utils.operate_debug_log( + test, params, session, vm, debug_log_operation + )And update call sites:
- session = create_service(session) - session = start_service(session) + session = create_service(session, vm) + session = start_service(session, vm) @@ - session = create_service(session) - session = start_service(session) + session = create_service(session, vm) + session = start_service(session, vm)Also applies to: 44-75, 144-146, 165-167, 174-175
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
🔇 Additional comments (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
168-173: LGTM: tuple/formatting bug fixedCorrectly uses cmd_status_output() return and %s formatting.
hellohellenmao
force-pushed
the
4086
branch
2 times, most recently
from
85204b4 to
100889b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
6-7: Fix virtiofsd version detection to include RELEASE (prevents false gating).Current command drops the RPM release, while the required range expects VERSION-RELEASE.
Apply:
- virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2 + virtiofsd_version_cmd = rpm -q --qf '%{VERSION}-%{RELEASE}\n' virtiofsd
🧹 Nitpick comments (7)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (7)
14-18: Harden pre_command and socket reuse.Make directory setup idempotent and avoid brittle seq usage; ensure stale socket is removed.
Apply:
- pre_command = "mkdir -p ${fs_source_dir} && cd ${fs_source_dir}" - pre_command += " && for i in $(seq 1 1024); do touch file_$i.txt; done" + pre_command = "set -e; mkdir -p ${fs_source_dir} && cd ${fs_source_dir}" + pre_command += " && for i in $(printf '%s\n' {1..1024}); do : > \"file_${i}.txt\"; done" + pre_command += " && rm -f ${virtiofs_socket_path}"Note: {1..1024} requires bash; if /bin/sh is used, switch to:
i=1; while [ $i -le 1024 ]; do : > "file_${i}.txt"; i=$((i+1)); done.
33-41: WinFSP install path likely reversed for x86_64.On 64‑bit Windows, 64‑bit WinFSP installs under “C:\Program Files” (x86 is for 32‑bit). Your check may false‑fail.
Apply:
- x86_64: - install_winfsp_path = 'C:\Program Files (x86)' + x86_64: + install_winfsp_path = 'C:\Program Files'If a 32‑bit package is intentionally used on x64, keep as-is and state rationale in a comment.
60-60: Do not override Windows list command with Linux ‘ls’. Scope per‑guest OS.The later
list_file_cmd = "ls %s"overwrites the Windows setting and will fail on Windows guests.Apply (scope by guest OS under each variant that needs it):
- list_file_cmd = "ls %s" + Linux: + list_file_cmd = "ls %s"Repeat for the 2048 variant as well.
Also applies to: 82-82, 97-97
67-70: Same brittleness on warning text (610 case).The “leaving only 0 file descriptors” portion is version‑dependent.
Apply:
- expected_msg = "File descriptor count limit is very small, leaving only 0 file descriptors for the guest" + expected_msg_regex = "File descriptor count limit is very small, leaving only \\d+ file descriptors for the guest"
85-98: 2048 case lacks success assertion; add a positive check.Validate that listing succeeds and that no “Too many open files” appears.
Apply:
- rlimit_nofile_2048: @@ - list_file_cmd = "ls %s" + Linux: + list_file_cmd = "ls %s" + # Expect success: no ENFILE/EMFILE symptoms + negative_qemu_log_pattern = "No more file descriptors available to the guest" + negative_ls_output_pattern = "Too many open files"Rename keys to whatever “negative_*” convention your harness supports (or invert with an allow‑list match).
16-18: Optional: make socket path unique per run to avoid collisions.Use a test‑scoped tag or PID in the path.
Example:
- virtiofs_socket_path = '/var/tmp/virtiofs_rlimit_nofile.sock' + virtiofs_socket_path = '/var/tmp/virtiofs_rlimit_nofile_${test_tag}.sock'
62-66: Avoid brittle exact-match for virtiofsd error (rlimit 512).Test reads params["expected_msg"] and the 512 branch does a substring check (qemu/tests/virtio_fs_rlimit_nofile.py, ~line 94); there is no existing expected_msg_regex key in the repo. Change the config variant instead of relying on a full sentence match — update qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (variant rlimit_nofile_512, line ~65) to a stable substring, e.g.
expected_msg = "Maximum number of file descriptors too small"
This is the minimal fix; alternatively add test support for an expected_msg_regex key if you prefer regex matching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/virtio_fs_rlimit_nofile.py
🔇 Additional comments (5)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (5)
1-5: LGTM: test gating and suite scaffolding look correctHost OS filters and test type definition are consistent with VirtioFS host-only constraints.
10-13: LGTM: fs source dir lifecycleAuto-create and cleanup help keep runs hermetic.
19-31: LGTM: shared memory backend for DAX/virtio-fsMemdev wiring and s390(x) exclusions are sane.
46-49: SC 'depend=' syntax — forward slashes are correctWindows sc expects dependency names separated by '/', so depend="WinFsp.Launcher/VirtioFsDrv" is valid; no change required.
71-85: *Keep _content keys; remove hard-coded counts from qemu_check_content.The test uses contains-style checks (see qemu/tests/virtio_fs_rlimit_nofile.py lines ~154–158), so renaming keys to *_pattern would break it — update qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg to set qemu_check_content to a stable substring, e.g. "No more file descriptors available to the guest", and leave ls_check_content as-is.
Likely an incorrect or invalid review comment.
hellohellenmao
force-pushed
the
4086
branch
from
100889b to
fc83349
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
qemu/tests/virtio_fs_rlimit_nofile.py (2)
88-92: Blocker: Unconditional params['cmd_run_virtiofsd'] breaks 1000/2048 flowsAccessing params['cmd_run_virtiofsd'] here raises KeyError for variants that don’t need it. Fetch inside 512/610 branches and validate presence.
Apply:
@@ - rlimit_nofile = params.get("rlimit_nofile") - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] - guest_mnts = dict() - os_type = params["os_type"] + rlimit_nofile = params.get("rlimit_nofile") + guest_mnts = dict() + os_type = params["os_type"]And in the 512/610 branches:
@@ - result = process.run( + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' in params for rlimit_nofile=512") + result = process.run( cmd_run_virtiofsd, shell=True, ignore_status=True, verbose=True, ) @@ - session = aexpect.ShellSession( + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.error("Missing 'cmd_run_virtiofsd' in params for rlimit_nofile=610") + session = aexpect.ShellSession( cmd_run_virtiofsd, auto_close=False, output_func=utils_misc.log_line, output_params=("virtiofs_fs-virtiofs.log",), prompt=r"^\[.*\][\#\$]\s*$", )Run to confirm cfg variants don’t define cmd_run_virtiofsd for 1000/2048:
#!/bin/bash rg -n "rlimit_nofile_(1000|2048)" -C3 --glob '!**/venv/**' rg -n "cmd_run_virtiofsd" --glob '!**/venv/**'
98-115: Blocker: bytes/str mismatch and stderr-only check may cause false negativesresult.stderr is likely bytes; comparing to str expected_msg can TypeError. Also errors may be on stdout. Decode and combine both streams.
Apply:
- # Prefer text-safe access for command output - status = result.exit_status - err_out = result.stderr + # Combine and decode both streams + status = getattr(result, "exit_status", result.exit_status) + stdout = getattr(result, "stdout", b"") + stderr = getattr(result, "stderr", b"") + if isinstance(stdout, (bytes, bytearray)): + stdout = stdout.decode(errors="ignore") + if isinstance(stderr, (bytes, bytearray)): + stderr = stderr.decode(errors="ignore") + out = f"{stdout}{stderr}" @@ - elif expected_msg not in err_out: + elif expected_msg not in out: test.fail( - "virtiofsd failed but without expected message. Output: %s" % err_out + "virtiofsd failed but without expected message. Output: %s" % out )
🧹 Nitpick comments (4)
qemu/tests/virtio_fs_rlimit_nofile.py (4)
119-139: Minor: Wording—don’t imply success when asserting an error indicationFor rlimit=610 you’re checking for an “expected message”; the log “started successfully” is misleading. Make it “observed expected message” to avoid confusion in reports.
- test.log.info( - "virtiofsd started successfully with the required message present" - ) + test.log.info("Observed expected message from virtiofsd (rlimit=610)")
79-86: Minor: Harden unmount cleanupIf utils_disk.umount fails, we silently continue. Consider checking return/exit status (or catching exceptions) and logging a warning so flakes are diagnosable.
140-179: Optional: Merge 1000 and 2048 paths to reduce driftBodies are nearly identical; collapse into one branch and vary validation.
Apply:
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info ) vm = env.get_vm(params.get("main_vm")) vm.verify_alive() session = vm.wait_for_login() try: session = create_service(session) session = start_service(session) - for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - test.log.debug("The dir output in guest: %s", out) - if params["ls_check_content"] not in out: - test.fail("Wrong content found: %s" % out) - qemu_out = vm.process.get_output() - if params["qemu_check_content"] not in qemu_out: - test.fail("Wrong qemu content found: %s" % qemu_out) + for fs_dest in guest_mnts.values(): + cmd = params["list_file_cmd"] % fs_dest + if rlimit_nofile == "1000": + out = session.cmd_output(cmd).strip() + test.log.debug("The dir output in guest: %s", out) + if params["ls_check_content"] not in out: + test.fail("Wrong content found: %s" % out) + qemu_out = vm.process.get_output() + if params["qemu_check_content"] not in qemu_out: + test.fail("Wrong qemu content found: %s" % qemu_out) + else: + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output) finally: stop_service(session) delete_service() - elif rlimit_nofile == "2048": - ...
93-180: Minor: Guard unknown rlimit valuesAdd an else: test.cancel(...) for unexpected rlimit_nofile to fail fast with a clear message.
@@ elif rlimit_nofile == "2048": ... + else: + test.cancel("Unsupported rlimit_nofile value: %s" % rlimit_nofile)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
hellohellenmao
force-pushed
the
4086
branch
2 times, most recently
from
61e2b5e to
7ba1a45
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
qemu/tests/virtio_fs_rlimit_nofile.py (3)
98-121: Make the 512 check robust: add timeout and search both stdout+stderrDaemons often log errors to stderr; add a timeout and combine both streams before matching.
- result = process.run( - cmd_run_virtiofsd, - shell=True, - ignore_status=True, - verbose=True, - ) - # Prefer text-safe access for command output (stderr for failure path) - status = result.exit_status - err_out = getattr(result, "stderr_text", None) - if err_out is None: - raw_err = getattr(result, "stderr", b"") - err_out = ( - raw_err.decode() - if isinstance(raw_err, (bytes, bytearray)) - else str(raw_err) - ) - if status == 0: + result = process.run( + cmd_run_virtiofsd, + shell=True, + ignore_status=True, + verbose=True, + timeout=int(params.get("virtiofsd_fail_timeout", 30)), + ) + status = result.exit_status + out_bytes = b"".join( + x + for x in [ + (getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b""), + ] + if x + ) + out = out_bytes.decode(errors="ignore") + if status == 0: test.fail( "virtiofsd unexpectedly started successfully with rlimit-nofile=512" ) - elif expected_msg not in err_out: + elif expected_msg not in out: test.fail( - "virtiofsd failed but without expected message. Output: %s" % err_out + "virtiofsd failed but without expected message. Output: %s" % out )
147-186: Reduce duplication: merge 1000 and 2048 branchesThe flows are identical except for validation. Merge to cut drift.
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info ) vm = env.get_vm(params.get("main_vm")) vm.verify_alive() session = vm.wait_for_login() try: session = create_service(session) session = start_service(session) for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - test.log.debug("The dir output in guest: %s", out) - if params["ls_check_content"] not in out: - test.fail("Wrong content found: %s" % out) - qemu_out = vm.process.get_output() - if params["qemu_check_content"] not in qemu_out: - test.fail("Wrong qemu content found: %s" % qemu_out) + if rlimit_nofile == "1000": + out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() + test.log.debug("The dir output in guest: %s", out) + if params["ls_check_content"] not in out: + test.fail("Wrong content found: %s" % out) + qemu_out = vm.process.get_output() + if params["qemu_check_content"] not in qemu_out: + test.fail("Wrong qemu content found: %s" % qemu_out) + else: + cmd = params["list_file_cmd"] % fs_dest + status, output = session.cmd_status_output(cmd) + output = output.strip() + if status != 0: + test.fail("list failed: %s" % output) finally: stop_service(session) delete_service() - - elif rlimit_nofile == "2048": - ...
88-92: Blocker: Unconditional params lookup will raise KeyError on 1000/2048 paths
params["cmd_run_virtiofsd"]is fetched unconditionally but only needed for 512/610 flows. This will crash 1000/2048 variants where the key isn’t present.Apply:
@@ - rlimit_nofile = params.get("rlimit_nofile") - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + rlimit_nofile = params.get("rlimit_nofile")And inside each branch that uses it:
@@ rlimit 512 block (before process.run) + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.cancel("Missing cmd_run_virtiofsd for rlimit_nofile=512")@@ rlimit 610 block (before ShellSession) + cmd_run_virtiofsd = params.get("cmd_run_virtiofsd") + if not cmd_run_virtiofsd: + test.cancel("Missing cmd_run_virtiofsd for rlimit_nofile=610")
🧹 Nitpick comments (2)
qemu/tests/virtio_fs_rlimit_nofile.py (2)
131-146: Ensure virtiofsd is terminated in 610 flow to avoid leaked processesAfter matching the message, explicitly interrupt/terminate the spawned process before closing.
finally: - session.close() + # Try to interrupt virtiofsd; ignore if already exited + try: + session.sendcontrol("c") + except Exception: + pass + session.close()Please confirm
ShellSession.close()guarantees child termination in your aexpect version; if not, considersession.kill()or starting viaprocess.SubProcess.
93-186: Guardrail: fail fast on unknown rlimit valuesAdd an
elseto cancel misconfigured runs.@@ - elif rlimit_nofile == "2048": + elif rlimit_nofile == "2048": ... - finally: - stop_service(session) - delete_service() + finally: + stop_service(session) + delete_service() + else: + test.cancel(f"Unsupported rlimit_nofile: {rlimit_nofile}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
🔇 Additional comments (2)
qemu/tests/virtio_fs_rlimit_nofile.py (2)
162-164: LGTM: fixed QEMU log check variableGood catch switching to
qemu_outfor both the check and the failure message.
79-83: Keep call as-is — utils_disk.umount signature is (src, dst[, fstype], session=...)Repo search shows canonical usage with (src, mount_point, fstype, session) (e.g. provider/disk_utils.py:128 and qemu/tests/qemu_guest_agent.py:3198); the call in qemu/tests/virtio_fs_rlimit_nofile.py matches that convention and does not need swapping.
Likely an incorrect or invalid review comment.
hellohellenmao
force-pushed
the
4086
branch
from
7ba1a45 to
0cb0611
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
6-7: Fix version check: include RELEASE in rpm query (prevents false negatives).
Current command drops the release. Align output with VERSION-RELEASE required by the range.- virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2 + virtiofsd_version_cmd = rpm -q --qf '%{VERSION}-%{RELEASE}\n' virtiofsd
🧹 Nitpick comments (6)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
14-15: Make pre_command fail-fast and validate file count.
Add a guard so the test fails if file creation is incomplete.- pre_command = "mkdir -p ${fs_source_dir} && cd ${fs_source_dir}" - pre_command += " && for i in $(seq 1 1024); do touch file_$i.txt; done" + pre_command = "set -e; mkdir -p ${fs_source_dir} && cd ${fs_source_dir}" + pre_command += " && for i in $(seq 1 1024); do : > file_$i.txt; done" + pre_command += " && test $(find . -maxdepth 1 -type f -name 'file_*.txt' | wc -l) -eq 1024"qemu/tests/virtio_fs_rlimit_nofile.py (5)
91-95: Defer params['cmd_run_virtiofsd'] lookup to branches (avoid KeyError in FS variants).
Safer when running only 1000/2048 paths.rlimit_nofile = params.get("rlimit_nofile") - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] guest_mnts = dict() os_type = params["os_type"]And inside each branch before first use:
@@ - result = process.run( + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + result = process.run(@@ - session = aexpect.ShellSession( + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + session = aexpect.ShellSession(
107-116: Search both stdout and stderr for the 512 failure text.
Some builds log errors to stdout. Combine streams to avoid false negatives.- status = result.exit_status - err_out = getattr(result, "stderr_text", None) - if err_out is None: - raw_err = getattr(result, "stderr", b"") - err_out = ( - raw_err.decode() - if isinstance(raw_err, (bytes, bytearray)) - else str(raw_err) - ) + status = result.exit_status + out_bytes = b"".join(x for x in [ + getattr(result, "stdout", b"") or b"", + getattr(result, "stderr", b"") or b"" + ] if x) + output = out_bytes.decode(errors="ignore") @@ - elif expected_msg not in err_out: + elif expected_msg not in output: test.fail( - "virtiofsd failed but without expected message. Output: %s" % err_out + "virtiofsd failed but without expected message. Output: %s" % output )Also applies to: 121-124
141-147: Make wait timeout configurable; 10s can be flaky.
Use a param with sane default.- session.read_until_any_line_matches([expected_msg], timeout=10) + timeout = int(params.get("virtiofsd_wait_timeout", "30")) + session.read_until_any_line_matches([expected_msg], timeout=timeout)
150-195: Deduplicate 1000/2048 branches (logic identical except assertion).
Reduce drift by merging into one branch keyed on rlimit value, with small conditional on the assertion.
195-195: Add trailing newline (style).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
| rlimit_nofile = 1000 | ||
| fs_binary_extra_options += " --rlimit-nofile ${rlimit_nofile}" | ||
| list_file_cmd = "ls %s" | ||
| qemu_check_content = "No more file descriptors available to the guest (0 available out of 390 initially), consider increasing the --rlimit-nofile value" |
There was a problem hiding this comment.
Regex vs literal: escape parentheses and make counts flexible.
The Python test uses re.search; unescaped parentheses won’t match the logged text and the exact numbers can vary. Use a regex with escaped parens and \d+.
- qemu_check_content = "No more file descriptors available to the guest (0 available out of 390 initially), consider increasing the --rlimit-nofile value"
+ qemu_check_content = r"No more file descriptors available to the guest \(\d+ available out of \d+ initially\), consider increasing the --rlimit-nofile value"📝 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.
| qemu_check_content = "No more file descriptors available to the guest (0 available out of 390 initially), consider increasing the --rlimit-nofile value" | |
| qemu_check_content = r"No more file descriptors available to the guest \(\d+ available out of \d+ initially\), consider increasing the --rlimit-nofile value" |
🤖 Prompt for AI Agents
In qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg around line 84, the test's
expected string is treated as a regex via re.search so unescaped parentheses and
fixed numbers will fail; update the pattern to escape the literal parentheses
and replace the exact counts with \d+ (one-or-more-digit placeholders) so the
regex matches variable numeric values and the parentheses in the logged message.
| log_file = os.path.join( | ||
| test.resultsdir, params.get("debug_log_file", "debug.log") | ||
| ) | ||
| with open(log_file, "r") as f: | ||
| out = f.read().strip() | ||
| m = re.search(params["qemu_check_content"], out, re.M) | ||
| if m is None: | ||
| test.fail("The qemu log does not correct, check please.") | ||
| finally: |
There was a problem hiding this comment.
Regex bug: parentheses in pattern aren’t escaped; match will fail.
Either treat the config value as a literal or allow an explicit regex param.
- out = f.read().strip()
- m = re.search(params["qemu_check_content"], out, re.M)
+ out = f.read().strip()
+ pattern = params.get("qemu_check_content_regex")
+ if not pattern:
+ pattern = re.escape(params["qemu_check_content"])
+ m = re.search(pattern, out, re.M)
if m is None:
- test.fail("The qemu log does not correct, check please.")
+ test.fail("Expected QEMU log not found. Pattern: %s" % pattern)📝 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.
| log_file = os.path.join( | |
| test.resultsdir, params.get("debug_log_file", "debug.log") | |
| ) | |
| with open(log_file, "r") as f: | |
| out = f.read().strip() | |
| m = re.search(params["qemu_check_content"], out, re.M) | |
| if m is None: | |
| test.fail("The qemu log does not correct, check please.") | |
| finally: | |
| log_file = os.path.join( | |
| test.resultsdir, params.get("debug_log_file", "debug.log") | |
| ) | |
| with open(log_file, "r") as f: | |
| out = f.read().strip() | |
| pattern = params.get("qemu_check_content_regex") | |
| if not pattern: | |
| pattern = re.escape(params["qemu_check_content"]) | |
| m = re.search(pattern, out, re.M) | |
| if m is None: | |
| test.fail("Expected QEMU log not found. Pattern: %s" % pattern) | |
| finally: |
🤖 Prompt for AI Agents
In qemu/tests/virtio_fs_rlimit_nofile.py around lines 165 to 173, the regex
pattern taken from params["qemu_check_content"] may contain unescaped
parentheses causing the search to fail; change the logic to treat the config
value as a literal by default (wrap it with re.escape before calling re.search)
and/or add an optional param (e.g. "qemu_check_is_regex" boolean) so when true
the value is used as a raw regex; update the call to re.search accordingly and
document the new param so existing configs keep working.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
qemu/tests/virtio_fs_rlimit_nofile.py (2)
92-92: KeyError risk: defer fetching cmd_run_virtiofsd to the branches that need it.Line 92 unconditionally fetches
cmd_run_virtiofsdfrom params using dictionary accessparams["cmd_run_virtiofsd"], which will raiseKeyErrorif the parameter is not defined. Looking at the code, only the 512 and 610 branches (lines 102, 135) actually use this variable, while the 1000 and 2048 branches rely on the filesystem harness and do not referencecmd_run_virtiofsdat all.Move the lookup into the 512 and 610 branches where it's actually needed.
Apply this diff:
rlimit_nofile = params.get("rlimit_nofile") - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] guest_mnts = dict() os_type = params["os_type"] if rlimit_nofile == "512": + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] expected_msg = params["expected_msg"]And:
elif rlimit_nofile == "610": + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] expected_msg = params["expected_msg"]
165-174: Regex pattern should be escaped or explicitly marked as regex.Line 170 uses
params["qemu_check_content"]directly as a regex pattern without escaping. If the config value contains regex metacharacters like parentheses, dots, or brackets, the search may fail or produce incorrect matches.Consider one of these solutions:
Solution 1 (Recommended): Treat as literal string by default
with open(log_file, "r") as f: out = f.read().strip() - m = re.search(params["qemu_check_content"], out, re.M) + pattern = re.escape(params["qemu_check_content"]) + m = re.search(pattern, out, re.M) if m is None: test.fail( "The qemu log does not correct, check %s please." % log_file )Solution 2: Add optional regex flag
with open(log_file, "r") as f: out = f.read().strip() - m = re.search(params["qemu_check_content"], out, re.M) + check_content = params["qemu_check_content"] + if not params.get("qemu_check_is_regex", False): + check_content = re.escape(check_content) + m = re.search(check_content, out, re.M) if m is None: test.fail( "The qemu log does not correct, check %s please." % log_file )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
| if rlimit_nofile == "512": | ||
| expected_msg = params["expected_msg"] | ||
| error_context.context( | ||
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | ||
| ) | ||
| result = process.run( | ||
| cmd_run_virtiofsd, | ||
| shell=True, | ||
| ignore_status=True, | ||
| verbose=True, | ||
| ) | ||
| # Prefer text-safe access for command output (stderr for failure path) | ||
| status = result.exit_status | ||
| err_out = getattr(result, "stderr_text", None) | ||
| if err_out is None: | ||
| raw_err = getattr(result, "stderr", b"") | ||
| err_out = ( | ||
| raw_err.decode() | ||
| if isinstance(raw_err, (bytes, bytearray)) | ||
| else str(raw_err) | ||
| ) | ||
| if status == 0: | ||
| test.fail( | ||
| "virtiofsd unexpectedly started successfully with rlimit-nofile=512" | ||
| ) | ||
| elif expected_msg not in err_out: | ||
| test.fail( | ||
| "virtiofsd failed but without expected message. Output: %s" % err_out | ||
| ) | ||
| error_context.context( | ||
| "virtiofsd failed as expected with the required message present", | ||
| test.log.info, | ||
| ) | ||
| elif rlimit_nofile == "610": | ||
| expected_msg = params["expected_msg"] | ||
| error_context.context( | ||
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | ||
| ) | ||
| session = aexpect.ShellSession( | ||
| cmd_run_virtiofsd, | ||
| auto_close=False, | ||
| output_func=utils_misc.log_line, | ||
| output_params=("virtiofs_fs-virtiofs.log",), | ||
| prompt=r"^\[.*\][\#\$]\s*$", | ||
| ) | ||
| try: | ||
| session.read_until_any_line_matches([expected_msg], timeout=10) | ||
| test.log.info( | ||
| "virtiofsd started successfully with the required message present" | ||
| ) | ||
| except aexpect.ExpectTimeoutError as e: | ||
| test.fail("Timeout for virtiofsd start with rlimit-nofile=610: %s" % e) | ||
| finally: | ||
| session.close() | ||
| elif rlimit_nofile == "1000": | ||
| error_context.context( | ||
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | ||
| ) | ||
| vm = env.get_vm(params.get("main_vm")) | ||
| vm.verify_alive() | ||
| session = vm.wait_for_login() | ||
| try: | ||
| session = create_service(session) | ||
| session = start_service(session) | ||
| for fs_dest in guest_mnts.values(): | ||
| # ls the dir in guest | ||
| out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() | ||
| test.log.debug("The dir output in guest: %s", out) | ||
| # check the qemu log whether there is the proper information | ||
| log_file = os.path.join( | ||
| test.resultsdir, params.get("debug_log_file", "debug.log") | ||
| ) | ||
| with open(log_file, "r") as f: | ||
| out = f.read().strip() | ||
| m = re.search(params["qemu_check_content"], out, re.M) | ||
| if m is None: | ||
| test.fail( | ||
| "The qemu log does not correct, check %s please." % log_file | ||
| ) | ||
| finally: | ||
| stop_service(session) | ||
| delete_service() | ||
| elif rlimit_nofile == "2048": | ||
| error_context.context( | ||
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | ||
| ) | ||
| vm = env.get_vm(params.get("main_vm")) | ||
| vm.verify_alive() | ||
| session = vm.wait_for_login() | ||
| try: | ||
| session = create_service(session) | ||
| session = start_service(session) | ||
| for fs_dest in guest_mnts.values(): | ||
| cmd = params["list_file_cmd"] % fs_dest | ||
| status, output = session.cmd_status_output(cmd) | ||
| output = output.strip() | ||
| if status != 0: | ||
| test.fail("list failed: %s" % output) | ||
| finally: | ||
| stop_service(session) | ||
| delete_service() |
There was a problem hiding this comment.
Add else clause to catch unexpected rlimit_nofile values.
The code handles four specific values ("512", "610", "1000", "2048") but has no else clause. If rlimit_nofile is set to an unexpected value (e.g., due to a typo in the config), the test will silently pass without executing any validation logic, potentially masking configuration errors.
Add an else clause after the final elif:
finally:
stop_service(session)
delete_service()
+ else:
+ test.error(
+ "Unsupported rlimit_nofile value: %s. "
+ "Expected one of: 512, 610, 1000, 2048" % rlimit_nofile
+ )📝 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.
| if rlimit_nofile == "512": | |
| expected_msg = params["expected_msg"] | |
| error_context.context( | |
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | |
| ) | |
| result = process.run( | |
| cmd_run_virtiofsd, | |
| shell=True, | |
| ignore_status=True, | |
| verbose=True, | |
| ) | |
| # Prefer text-safe access for command output (stderr for failure path) | |
| status = result.exit_status | |
| err_out = getattr(result, "stderr_text", None) | |
| if err_out is None: | |
| raw_err = getattr(result, "stderr", b"") | |
| err_out = ( | |
| raw_err.decode() | |
| if isinstance(raw_err, (bytes, bytearray)) | |
| else str(raw_err) | |
| ) | |
| if status == 0: | |
| test.fail( | |
| "virtiofsd unexpectedly started successfully with rlimit-nofile=512" | |
| ) | |
| elif expected_msg not in err_out: | |
| test.fail( | |
| "virtiofsd failed but without expected message. Output: %s" % err_out | |
| ) | |
| error_context.context( | |
| "virtiofsd failed as expected with the required message present", | |
| test.log.info, | |
| ) | |
| elif rlimit_nofile == "610": | |
| expected_msg = params["expected_msg"] | |
| error_context.context( | |
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | |
| ) | |
| session = aexpect.ShellSession( | |
| cmd_run_virtiofsd, | |
| auto_close=False, | |
| output_func=utils_misc.log_line, | |
| output_params=("virtiofs_fs-virtiofs.log",), | |
| prompt=r"^\[.*\][\#\$]\s*$", | |
| ) | |
| try: | |
| session.read_until_any_line_matches([expected_msg], timeout=10) | |
| test.log.info( | |
| "virtiofsd started successfully with the required message present" | |
| ) | |
| except aexpect.ExpectTimeoutError as e: | |
| test.fail("Timeout for virtiofsd start with rlimit-nofile=610: %s" % e) | |
| finally: | |
| session.close() | |
| elif rlimit_nofile == "1000": | |
| error_context.context( | |
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | |
| ) | |
| vm = env.get_vm(params.get("main_vm")) | |
| vm.verify_alive() | |
| session = vm.wait_for_login() | |
| try: | |
| session = create_service(session) | |
| session = start_service(session) | |
| for fs_dest in guest_mnts.values(): | |
| # ls the dir in guest | |
| out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() | |
| test.log.debug("The dir output in guest: %s", out) | |
| # check the qemu log whether there is the proper information | |
| log_file = os.path.join( | |
| test.resultsdir, params.get("debug_log_file", "debug.log") | |
| ) | |
| with open(log_file, "r") as f: | |
| out = f.read().strip() | |
| m = re.search(params["qemu_check_content"], out, re.M) | |
| if m is None: | |
| test.fail( | |
| "The qemu log does not correct, check %s please." % log_file | |
| ) | |
| finally: | |
| stop_service(session) | |
| delete_service() | |
| elif rlimit_nofile == "2048": | |
| error_context.context( | |
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | |
| ) | |
| vm = env.get_vm(params.get("main_vm")) | |
| vm.verify_alive() | |
| session = vm.wait_for_login() | |
| try: | |
| session = create_service(session) | |
| session = start_service(session) | |
| for fs_dest in guest_mnts.values(): | |
| cmd = params["list_file_cmd"] % fs_dest | |
| status, output = session.cmd_status_output(cmd) | |
| output = output.strip() | |
| if status != 0: | |
| test.fail("list failed: %s" % output) | |
| finally: | |
| stop_service(session) | |
| delete_service() | |
| elif rlimit_nofile == "2048": | |
| error_context.context( | |
| "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info | |
| ) | |
| vm = env.get_vm(params.get("main_vm")) | |
| vm.verify_alive() | |
| session = vm.wait_for_login() | |
| try: | |
| session = create_service(session) | |
| session = start_service(session) | |
| for fs_dest in guest_mnts.values(): | |
| cmd = params["list_file_cmd"] % fs_dest | |
| status, output = session.cmd_status_output(cmd) | |
| output = output.strip() | |
| if status != 0: | |
| test.fail("list failed: %s" % output) | |
| finally: | |
| stop_service(session) | |
| delete_service() | |
| else: | |
| test.error( | |
| "Unsupported rlimit_nofile value: %s. " | |
| "Expected one of: 512, 610, 1000, 2048" % rlimit_nofile | |
| ) |
🤖 Prompt for AI Agents
In qemu/tests/virtio_fs_rlimit_nofile.py around lines 96 to 196, the branching
handles only "512", "610", "1000", and "2048" but lacks a final else to catch
unexpected rlimit_nofile values; add an else clause after the last elif that
logs the unexpected value and fails the test (e.g., test.fail("Unexpected
rlimit_nofile value: %s; expected one of: 512, 610, 1000, 2048" %
rlimit_nofile)) so misconfigurations don’t silently pass.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
qemu/tests/virtio_fs_rlimit_nofile.py (4)
89-89: Defer cmd_run_virtiofsd lookup to branches that use itPre-fetching can raise KeyError for variants that don’t need it and tightens coupling to cfg. Fetch inside the 512/610 paths right before use.
Apply:
- cmd_run_virtiofsd = params["cmd_run_virtiofsd"] @@ - result = process.run( + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + result = process.run( cmd_run_virtiofsd, shell=True, ignore_status=True, verbose=True, ) @@ - session = aexpect.ShellSession( + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] + session = aexpect.ShellSession( cmd_run_virtiofsd, auto_close=False, output_func=utils_misc.log_line, output_params=("virtiofs_fs-virtiofs.log",), prompt=r"^\[.*\][\#\$]\s*$", )Also applies to: 98-103, 131-137
104-121: Search both stdout and stderr for the expected failure textDaemons often log errors to stderr; sometimes stdout. Combine both to avoid false negatives.
Apply:
- # Prefer text-safe access for command output (stderr for failure path) - status = result.exit_status - err_out = getattr(result, "stderr_text", None) - if err_out is None: - raw_err = getattr(result, "stderr", b"") - err_out = ( - raw_err.decode() - if isinstance(raw_err, (bytes, bytearray)) - else str(raw_err) - ) - if status == 0: + status = result.exit_status + out_bytes = b"".join( + x for x in [ + (getattr(result, "stdout", b"") or b""), + (getattr(result, "stderr", b"") or b""), + ] if isinstance(x, (bytes, bytearray)) + ) + out = out_bytes.decode(errors="ignore") if out_bytes else " ".join( + x for x in [ + getattr(result, "stdout_text", None), + getattr(result, "stderr_text", None), + ] if x + ) + if status == 0: test.fail( "virtiofsd unexpectedly started successfully with rlimit-nofile=512" ) - elif expected_msg not in err_out: + elif expected_msg not in out: test.fail( - "virtiofsd failed but without expected message. Output: %s" % err_out + "virtiofsd failed but without expected message. Output: %s" % out )
147-185: Deduplicate 1000/2048 branches to reduce driftLogic is identical except the assertion. Merge into one branch keyed by value.
Example:
- elif rlimit_nofile == "1000": + elif rlimit_nofile in ("1000", "2048"): ... - for fs_dest in guest_mnts.values(): - out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() - ... + for fs_dest in guest_mnts.values(): + if rlimit_nofile == "1000": + out = session.cmd_output(params["list_file_cmd"] % fs_dest).strip() + test.log.debug("The dir output in guest: %s", out) + if params.get("ls_check_content") not in out: + test.fail("The list output is not proper: %s" % out) + else: + status, output = session.cmd_status_output(params["list_file_cmd"] % fs_dest) + if status != 0: + test.fail("list failed: %s" % output.strip())
147-185: Add a final else to guard unexpected rlimit_nofile valuesUnrecognized values currently no-op, causing false-passing tests.
Apply:
finally: stop_service(session) delete_service() + else: + test.error( + "Unsupported rlimit_nofile value: %s. Expected one of: 512, 610, 1000, 2048" + % rlimit_nofile + )qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
5-6: Version gating is incorrect: query VERSION-RELEASE, not just VERSION
cut -d- -f2yields only VERSION, butrequired_virtiofsd_version = [1.13.2-1,)expects VERSION-RELEASE. Use rpm queryformat.Apply:
- virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2 + virtiofsd_version_cmd = rpm -q --qf '%{VERSION}-%{RELEASE}\n' virtiofsd
🧹 Nitpick comments (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (2)
82-83: Make the ls error check resilient across distros/l10n“Too many open files in system” can differ by distro or locale. Consider validating by non-zero exit status and/or allow a regex param (e.g., ls_check_regex) instead of a fixed literal.
15-16: Avoid socket path collisions in parallel runsStatic socket path can collide across concurrent jobs. Prefer a per-job unique path (e.g., append job id or use mktemp).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (5)
create_viofs_service(318-363)delete_viofs_serivce(366-385)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
6-6: Version check will under-report: include release in rpm query.The
cut -d- -f2returns only VERSION (e.g., 1.13.2), but Line 5 specifiesrequired_virtiofsd_version = [1.13.2-1,)which expects VERSION-RELEASE format. This will cause the version check to fail even when a valid version is installed.Apply this diff:
- virtiofsd_version_cmd = rpm -q virtiofsd | cut -d- -f2 + virtiofsd_version_cmd = rpm -q --qf '%{VERSION}-%{RELEASE}\n' virtiofsdqemu/tests/virtio_fs_rlimit_nofile.py (2)
89-89: Avoid KeyError: defer fetching cmd_run_virtiofsd to branches that use it.The unconditional lookup
cmd_run_virtiofsd = params["cmd_run_virtiofsd"]on Line 89 will raise KeyError when running the 1000/2048 variants, which rely on the fs harness (filesystems parameter) instead of directly invoking virtiofsd. The configuration file doesn't definecmd_run_virtiofsdfor these variants.Apply this diff:
rlimit_nofile = params.get("rlimit_nofile") - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] guest_mnts = dict() os_type = params["os_type"] if rlimit_nofile == "512": expected_msg = params["expected_msg"] error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info ) + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] result = process.run(And similarly for the 610 branch:
elif rlimit_nofile == "610": expected_msg = params["expected_msg"] error_context.context( "Starting virtiofsd with rlimit-nofile=%s" % rlimit_nofile, test.log.info ) + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] session = aexpect.ShellSession(
93-185: Add else clause to catch unexpected rlimit_nofile values.The code handles four specific values ("512", "610", "1000", "2048") but lacks an else clause. If
rlimit_nofileis set to an unexpected value (e.g., due to a typo in the config), the test will silently pass without executing any validation logic, potentially masking configuration errors.Add an else clause after the final elif:
finally: stop_service(session) delete_service() + else: + test.error( + "Unsupported rlimit_nofile value: %s. " + "Expected one of: 512, 610, 1000, 2048" % rlimit_nofile + )
🧹 Nitpick comments (1)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg (1)
69-84: Consider adding QEMU log validation for the 1000 variant.The 1000 variant verifies the guest error message via
ls_check_content, but doesn't validate the corresponding QEMU/virtiofsd log output. For comprehensive testing of RLIMIT_NOFILE behavior, you may want to add aqemu_check_contentparameter to verify that virtiofsd logs the appropriate warning when running out of file descriptors.Example addition after line 82:
qemu_check_content = r"No more file descriptors available to the guest \(\d+ available out of \d+ initially\)"Note: If you add this parameter, you'll also need to update the Python test to read and validate the QEMU process output.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (5)
create_viofs_service(318-363)delete_viofs_serivce(366-385)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
1. Create a shared directory for testing on the host. 2. Touch 1024 files in the shared directory. 3. Start the virtiofsd daemon with rlimit-nofile and check. Signed-off-by: Tingting Mao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
qemu/tests/virtio_fs_rlimit_nofile.py (2)
93-185: Add else clause to catch unexpected rlimit_nofile values.The code handles only "512", "610", "1000", and "2048" but has no else clause. If
rlimit_nofileis set to an unexpected value (e.g., due to a typo in the config), the test will silently pass without executing any validation logic, potentially masking configuration errors.Add an else clause after the final elif:
finally: stop_service(session) delete_service() + else: + test.error( + "Unsupported rlimit_nofile value: %s. " + "Expected one of: 512, 610, 1000, 2048" % rlimit_nofile + )
89-89: Unconditional param fetch will break 1000/2048 variants.Line 89 fetches
cmd_run_virtiofsdunconditionally, but this parameter is only used in the 512 and 610 branches. The 1000 and 2048 variants rely on filesystem harness flags and will raise aKeyErrorif this parameter is missing from their configuration.Move the fetch into the branches that require it:
rlimit_nofile = params.get("rlimit_nofile") - cmd_run_virtiofsd = params["cmd_run_virtiofsd"] guest_mnts = dict() os_type = params["os_type"] if rlimit_nofile == "512": + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] expected_msg = params["expected_msg"]elif rlimit_nofile == "610": + cmd_run_virtiofsd = params["cmd_run_virtiofsd"] expected_msg = params["expected_msg"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg(1 hunks)qemu/tests/virtio_fs_rlimit_nofile.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/virtio_fs_rlimit_nofile.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/virtio_fs_rlimit_nofile.py (1)
provider/virtio_fs_utils.py (6)
create_viofs_service(318-363)delete_viofs_serivce(366-385)operate_debug_log(478-514)start_viofs_service(388-406)get_virtiofs_driver_letter(13-29)stop_viofs_service(433-446)
🔇 Additional comments (8)
qemu/tests/virtio_fs_rlimit_nofile.py (8)
37-42: Verify session lifecycle in delete_service.The
delete_servicefunction creates a fresh session viavm.wait_for_login()at line 40 and closes it at line 42, rather than reusing an existing session parameter likecreate_serviceandstart_servicedo. This inconsistency may indicate either:
- An intentional design (cleanup uses a fresh session), or
- A missed refactoring opportunity (should accept and reuse session parameter)
Please confirm whether this session management pattern is intentional. If
delete_serviceis always called in a finally block after the session may be stale or closed, then creating a fresh session makes sense. Otherwise, consider accepting a session parameter for consistency:def delete_service(session=None): if os_type == "windows": error_context.context("Delete virtiofs service in guest.", test.log.info) if session is None: session = vm.wait_for_login() virtio_fs_utils.delete_viofs_serivce(test, params, session) session.close()
22-35: LGTM: create_service handles OS-specific logic correctly.The function appropriately checks
os_typeand only creates the Windows service when needed. The driver verifier check and service creation flow align with the utility functions invirtio_fs_utils.
44-74: LGTM: start_service correctly branches by OS.The Linux path mounts the virtiofs filesystem and the Windows path starts the service and retrieves the driver letter. Both paths correctly update
guest_mntsand return the session.
76-86: LGTM: stop_service handles cleanup correctly.The function properly unmounts Linux filesystems or stops the Windows service based on
os_type, and safely closes the session.
93-125: LGTM: 512 branch correctly validates failure path.The branch runs virtiofsd with
rlimit-nofile=512, expects it to fail, and verifies the error message is present in stderr. The defensive attribute access forstderr_textwith fallback tostderris robust.
126-146: LGTM: 610 branch correctly validates startup message.The branch launches virtiofsd interactively via
aexpect.ShellSessionand waits for the expected message with a 10-second timeout. Thefinallyblock ensures the session is always closed.
147-166: LGTM: 1000 branch validates guest mount and content.The branch correctly creates/starts the service, verifies the listing output, and ensures cleanup via the
finallyblock. The safeparams.get()call on line 162 prevents KeyErrors.
167-185: LGTM: 2048 branch validates listing command exit status.The branch correctly unpacks
cmd_status_output, strips the output, and fails if the listing command returns non-zero. Cleanup is properly handled in thefinallyblock.
| fs = params["filesystems"] | ||
| fs_params = params.object_params(fs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard against missing filesystems parameter.
Lines 45-46 fetch filesystems from params without error handling. If this parameter is missing or misconfigured, the test will crash with an unhelpful KeyError instead of providing clear context.
Add defensive handling:
def start_service(session):
- fs = params["filesystems"]
+ fs = params.get("filesystems")
+ if not fs:
+ test.error("filesystems parameter is required but not found in config")
fs_params = params.object_params(fs)📝 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.
| fs = params["filesystems"] | |
| fs_params = params.object_params(fs) | |
| def start_service(session): | |
| fs = params.get("filesystems") | |
| if not fs: | |
| test.error("filesystems parameter is required but not found in config") | |
| fs_params = params.object_params(fs) |
🤖 Prompt for AI Agents
In qemu/tests/virtio_fs_rlimit_nofile.py around lines 45-46, the code directly
accesses params["filesystems"] which raises an unhelpful KeyError if the
parameter is missing; update this to defensively handle missing or misconfigured
input by first checking for the key (e.g., use params.get("filesystems") or a
try/except KeyError), and if absent log or raise a clear, contextual error
message (or skip the test) explaining that the 'filesystems' parameter is
required; then only call params.object_params(fs) when a valid fs value is
present.
|
Test pass for windows: |
1 participant
ID: 4086
Summary by CodeRabbit