virtio_fs_rlimit_nofile: new case(WIP) #4376
Conversation
WalkthroughAdds a Linux-only VirtioFS RLIMIT_NOFILE test: a new QEMU config and Python test that exercise virtiofsd with four rlimit values (512, 610, 1000, 2048), validating virtiofsd startup failures, log messages, and guest-mounted filesystem behavior with service lifecycle management. 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 over Runner,Host: Variant rlimit_nofile=512
Runner->>Host: run cmd_run_virtiofsd --rlimit-nofile 512 (process.run)
Host-->>Runner: non-zero exit + output
Runner->>Runner: assert output contains expected_msg
end
rect rgba(230,255,230,0.6)
note over Runner,Shell: Variant rlimit_nofile=610
Runner->>Shell: start interactive session (aexpect)
Shell->>virtiofsd: launch --rlimit-nofile 610
virtiofsd-->>Shell: logs stream
Shell-->>Runner: output forwarded
Runner->>Runner: wait ≤10s for expected_msg
Runner->>Shell: close session
end
rect rgba(255,245,230,0.6)
note over Runner,VM: Variants rlimit_nofile=1000 / 2048
Runner->>VM: verify VM alive & get 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🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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
from
fc83349 to
61e2b5e
Compare
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]>
hellohellenmao
force-pushed
the
4086
branch
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.
ID: 4086
Summary by CodeRabbit