Fix the issue when var is empty but not None#4199
Fix the issue when var is empty but not None#4199YvanY0 merged 1 commit intoavocado-framework:masterfrom
Conversation
leidwang
changed the title
|
Hi @PaulYuuu @fbq815 Could you please help review this MR?Thanks. |
| LOG.info("Output of findmnt: %s", mount_result) | ||
| except process.CmdError: | ||
| pass | ||
| except Exception as e: |
There was a problem hiding this comment.
session.cmd_output_safe will not raise exception if the status is not zero, please use session.cmd instead. and here should be process.CmdError, aexpect.exceptions.ShellCmdError
There was a problem hiding this comment.
Oh, yes, forgot to update that, thanks!
|
Hello @YongxueHong @qingwangrh @hellohellenmao, this is a regression from #4098, please review and verify the fix, thanks. |
| except process.CmdError: | ||
| pass | ||
| except Exception as e: | ||
| LOG.info("Exception info: %s", e) |
There was a problem hiding this comment.
| LOG.info("Exception info: %s", e) | |
| LOG.error("Exception info: %s", e) |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
LGTM |
fbq815
left a comment
fbq815
left a comment
There was a problem hiding this comment.
LGTM, details in jira ticket
|
Hi @smitterl |
|
The relevant patchs make a regression issue https://issues.redhat.com/browse/RHEL-108081 http://10.73.156.41/pub/logs/qinwang/qbugs/p108081/4199/2025-08-08/ |
|
change findmnt supports finding by src or dst. |
@PaulYuuu Could you please share your thoughts about that? Thanks. |
I was also mentioned this in the previous PR: #4098 (comment) |
Hi @qingwangrh |
Yes, but we (Yongxue, Yihuang, and Leidong) discussed this issue at a Google meeting before. I'm not sure why we decided not to change it last time, but we did discuss it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
virttest/utils_disk.py (5)
68-83: Signature made src optional and added empty-args guard — refine docstring and error messageThe behavior change is correct for dst-only searches. Please tighten docs and make the error message clearer.
Apply this diff to improve clarity:
def is_mount(src=None, dst=None, fstype=None, options=None, verbose=False, session=None): """ - Check is src or dst mounted. + Check whether src and/or dst is mounted using findmnt. - :param src: source device or directory - :param dst: mountpoint, if None will skip to check - :param fstype: file system type, if None will skip to check - :param options: mount options should be separated by "," - :param session: check within the session if given + :param src: source device or directory; if None, skip src criterion + :param dst: mountpoint; if None, skip dst criterion + :param fstype: file system type; if None, skip fstype criterion + :param options: findmnt -O filter string (comma-separated) + :param verbose: when True, log command output and status + :param session: check within the given session, if provided - :return: True if mounted, else return False + :return: True if mounted, else False """ @@ - if mount_opts == "": - raise exceptions.TestError("Mount options is empty, it is meaningless") + if mount_opts == "": + raise exceptions.TestError( + "No query parameters provided to findmnt; set at least one of src, dst, fstype, or options" + )
81-81: Quote findmnt argument values to handle whitespace and reduce shell risksQuoting values avoids breakage on paths with spaces and reduces injection risk.
Apply this diff:
- mount_opts = " ".join(f"{opt} {val}" for opt, val in mount_options if val) + mount_opts = " ".join(f"{opt} {shlex.quote(str(val))}" for opt, val in mount_options if val)Add the missing import near the other stdlib imports:
import shlex
94-96: Exception handling broadened — add context about the commandCurrent logging prints only the exception string. Including the command greatly helps troubleshooting.
Apply this diff:
- except (process.CmdError, aexpect.exceptions.ShellCmdError) as e: - LOG.error("Exception info: %s", e) + except (process.CmdError, aexpect.exceptions.ShellCmdError) as e: + LOG.error("Exception running '%s': %s", mount_check_cmd, e) return False
99-104: Logging src when it may be NoneWhen src is None, logs show “None is mounted”. Prefer logging whichever identifier is provided.
Apply this diff:
- if verbose: - LOG.info("%s is mounted", src) + if verbose: + LOG.info("%s is mounted", src or dst) return True if verbose: - LOG.info("%s is not mounted", src) + LOG.info("%s is not mounted", src or dst)
68-96: Prefer status-based result over output truthinessfindmnt signals matches via exit status; you already catch non-zero and return False. You can simplify by returning True after a successful run, instead of checking output truthiness.
Apply this minimal diff:
try: if session: mount_result = session.cmd(mount_check_cmd) else: mount_result = process.run(mount_check_cmd, shell=True).stdout_text if verbose: LOG.info("Output of findmnt: %s", mount_result) + return True except (process.CmdError, aexpect.exceptions.ShellCmdError) as e: LOG.error("Exception info: %s", e) return False - - if mount_result: - if verbose: - LOG.info("%s is mounted", src) - return True - if verbose: - LOG.info("%s is not mounted", src) - return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/utils_disk.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
virttest/utils_disk.py (5)
virttest/utils_libguestfs.py (2)
cmd(392-403)run(655-662)virttest/remote_commander/remote_master.py (1)
cmd(326-339)virttest/qemu_monitor.py (2)
cmd(931-981)cmd(2033-2093)virttest/ovs_utils.py (1)
cmd(36-40)virttest/remote_commander/remote_runner.py (1)
shell(660-676)
🪛 GitHub Actions: CI
virttest/utils_disk.py
[error] 63-75: Black formatting would reformat 1 file; 'make check' failed due to formatting issues in virttest/utils_disk.py. Run 'black .' or 'make format' to fix.
🔇 Additional comments (3)
virttest/utils_disk.py (3)
20-20: Import aexpect to catch ShellCmdError — goodThis aligns with the broadened exception handling you added below.
89-91: Using session.cmd is correctThis matches the earlier review guidance; session.cmd raises on non-zero, which we handle below.
1-1755: Ensure Black formatting is appliedI was unable to run
make formatorblack .in this environment. Please run the formatter locally to resolve any Black formatting issues invirttest/utils_disk.pybefore merging.
Currently if var is empty but not None it will cause findmnt cmd run into error, so update the code logic to fix this issue. Also add some log output for the exception. Signed-off-by: Leidong Wang <leidwang@redhat.com>
If you design the src and dst as parameters, it means the user needs to care which parameter is passed. |
|
After applying this patch, the block related cases passed. |
YongxueHong
requested review from
Yingshun,
chunfuwen,
dzhengfy,
nanli1 and
smitterl
|
Hi @dzhengfy @chunfuwen @nanli1 @Yingshun @smitterl |
|
#4214 is invalid and can be solved by the current PR |
|
This patch fixed the empty string issue for block job loop: (025/225) Host_RHEL.m9.u7.ovmf.qcow2.virtio_blk.up.virtio_net.Guest.RHEL.9.7.0.x86_64.io-github-autotest-qemu.blockdev_stream_filter_nodename.q35: STARTED Got L0097 ERROR| Exception info: Shell command failed: 'findmnt -J -S /dev/vdb1 -M /mnt/vdb1 -t ext4' (status: 1, output: '') in log file ACK |
|
After applying this patch, the usb_multi_disk related cases passed. (1/2) Host_RHEL.m10.u0.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.0.x86_64.io-github-autotest-qemu.usb.usb_multi_disk.max_disk.without_usb_hub.nec-xhci.q35: STARTED |
I saw @qingwangrh opened a new PR to fix some regression issues autotest/tp-qemu#4357, which was caused by setting src=None. I am not sure if there are other places where such changes are needed? Or can we keep the previous usage/definition of |
|
This patch fixes the mount failure issue in the migration job. (1/1) type_specific.io-github-autotest-libvirt.migration_with_copy_storage.migration_retain_sparsity.disk_num_1.dest1_raw.src1_qcow2.copy_storage_all: STARTED |
|
@chunfuwen hi chunfu , Maybe you need to check if this pr passed on virtual disk test cases |
We cannot say it's a regression from the previous PR, because |
Oh, got it, I misunderstood it before.Thanks. |
Not worry, we can double confirm with @qingwangrh |
I mean the regression is the relevant patch: #4098 mount_str = "%s %s %s" % (src, dst, fstype) for result in mount_result.splitlines(): =========================== The caller must care the src/dst in the present implementation. You need not to concern the other place change. It helps to correct the wrong usage. But if you want to support fuzzy search or tolerate wrong usage, you may change to mount_options = [ ("-t", fstype), ("-O", options)] if src and dst: mount_opts =mount_opts +"".join(f"{opt} {val}" for opt, val in mount_options if val) if not mount_opts : mount_check_cmd = f"findmnt {mount_opts} -J" ..... (PS: you may decide the final solution.) |
After replying this PR, 1 failed libvirt virtual disk case virtual_disks.vhostvdpa.lifecycle can be passed now. |
dzhengfy
left a comment
dzhengfy
left a comment
There was a problem hiding this comment.
I think this increase the adaption scope, so it is low risk for existing code. LGTM
13 participants
Currently if var is empty but not None it will cause findmnt cmd run into error, so update the code logic to fix this issue. Also add some log output for the exception.
ID:4169
Summary by CodeRabbit
Bug Fixes
Behavior Changes