utils_disk: Add a default mount option#4214
utils_disk: Add a default mount option#4214cliping wants to merge 1 commit intoavocado-framework:masterfrom
Conversation
📝 WalkthroughWalkthroughThe change updates the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/utils_disk.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
virttest/utils_disk.py
[error] 1100-1120: Black formatting check failed. The file would be reformatted. Run 'black avocado-vt/virttest/utils_disk.py' to fix code style issues.
🔇 Additional comments (1)
virttest/utils_disk.py (1)
1115-1116: Centralize mount() defaults but review all call sitesBefore applying the targeted fix in utils_disk.mount(), note that we’ve identified numerous other invocations of
mount()without an explicitoptions=argument. Defaulting empty options to"rw"will inject-o rwinto each of these calls, which may subtly change behavior in different contexts. Please review whether a global"rw"default is appropriate for all of them or if specific call sites need explicit overrides.Call sites without
options=(excerpt):
- virttest/nfs.py:225 (
utils_misc.mount(self.mount_src, self.mount_dir, "nfs", perm=self.mount_options))- virttest/utils_libguestfs.py:708, 766
- virttest/utils_disk.py:775, 824, 1671
- virttest/vt_utils/filesystem.py:64, 199, 278
- virttest/utils_v2v.py:1652
- virttest/utils_test/init.py:711
- virttest/gluster.py:252
Proposed global refactor in
virttest/utils_disk.py:@@ def mount(src, dst, fstype=None, options=None, verbose=False, session=None): - # Normalize options; keep empty string internal but avoid passing empty "-O" to findmnt - options = options or "" + # Normalize options: skip findmnt filter if empty, then default to "rw" for actual mount + options = options or None - mounted = is_mount(src, dst, fstype, options if options else None, verbose, session) + mounted = is_mount(src, dst, fstype, options, verbose, session) - if mounted and "remount" not in options: - options = "remount" if not options else f"remount,{options}" + if mounted and (not options or "remount" not in options): + options = "remount" if not options else f"remount,{options}" - # If options is still empty, apply default "rw" to align with PR objective - eff_options = options or "rw" - if eff_options: - cmd.extend(["-o", eff_options]) + # Always provide an -o flag: default to "rw" when options is None + cmd.extend(["-o", options or "rw"])Please confirm the default-
rwbehavior is acceptable across all listed call sites or adjust those that require different mount options.
In mount(), if there are no mount option, is_mount() will always return True. So add a default mount option in configure_empty_linux_disk(). Signed-off-by: lcheng <lcheng@redhat.com>
|
Since 4199 can fix this issue. So close this PR. |
1 participant
In mount(), if there are no mount option, is_mount() will always return True. So add a default mount option.
Before:
[stdlog] 2025-08-10 11:30:45,881 aexpect.client client L1412 DEBUG| Sending command (safe): findmnt -J -S /dev/sda1 -M /mnt/sda1 -t ext4 -O
[stdlog] 2025-08-10 11:30:45,986 avocado.virttest.utils_disk utils_disk L0094 INFO | Output of findmnt: findmnt: option requires an argument -- 'O'
[stdlog] Try 'findmnt --help' for more information.
[stdlog] 2025-08-10 11:30:45,986 avocado.virttest.utils_disk utils_disk L0100 INFO | /dev/sda1 is mounted
[stdlog] 2025-08-10 11:30:45,986 aexpect.client client L1412 DEBUG| Sending command (safe): mount -t ext4 -o remount, /dev/sda1 /mnt/sda1
[stdlog] 2025-08-10 11:30:46,091 aexpect.client client L1412 DEBUG| Sending command (safe): echo $?
After:
[stdlog] 2025-08-10 12:07:27,419 aexpect.client client L1412 DEBUG| Sending command (safe): findmnt -J -S /dev/sda1 -M /mnt/sda1 -t ext4 -O rw
[stdlog] 2025-08-10 12:07:27,524 avocado.virttest.utils_disk utils_disk L0094 INFO | Output of findmnt:
[stdlog] 2025-08-10 12:07:27,524 avocado.virttest.utils_disk utils_disk L0103 INFO | /dev/sda1 is not mounted
[stdlog] 2025-08-10 12:07:27,524 aexpect.client client L1412 DEBUG| Sending command (safe): mount -t ext4 -o rw /dev/sda1 /mnt/sda1
(1/1) type_specific.io-github-autotest-libvirt.migration_with_copy_storage.migration_retain_sparsity.disk_num_1.dest1_qcow2.src1_qcow2.copy_storage_all: STARTED
(1/1) type_specific.io-github-autotest-libvirt.migration_with_copy_storage.migration_retain_sparsity.disk_num_1.dest1_qcow2.src1_qcow2.copy_storage_all: PASS (213.17 s)
Summary by CodeRabbit