Open
Conversation
WalkthroughAdds cache and sync options for disk devices (virtio-blk, nvme, usb-mass-storage): new enum types and validators, DiskStorageConfig fields, CLI parsing and ToCmdLine serialization, type-specific validation, mapping to VZ enums, doc updates, and expanded tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (--device args)
participant Config as DiskStorageConfig
participant Validator as validate()
participant Converter as toVz()
participant VZ as VZ runtime
CLI->>Config: parse `type`, `path`, `cache`, `sync`
Config->>Validator: validate type-specific rules (no cache for dev, no fsync for dev)
Validator-->>Config: ok / error
Config->>Converter: map caching/sync enums to VZ enums
Converter-->>VZ: produce VZ disk config
VZ-->>CLI: attach disk / return error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/config/virtio.go (1)
1002-1025:⚠️ Potential issue | 🟠 MajorValidate
cache/syncbefore serializing.Right now the new restrictions only run on the
FromOptions()path. BecauseDiskStorageConfigis public, callers can populateType,CachingMode, orSynchronizationModedirectly andToCmdLine()will emit combinations that the parser would reject.Suggested hardening
func (config *DiskStorageConfig) ToCmdLine() ([]string, error) { if config.ImagePath == "" { return nil, fmt.Errorf("%s devices need the path to a disk image", config.DevName) } + if !config.Type.IsValid() { + return nil, fmt.Errorf("unexpected value for disk 'type' option: %s", config.Type) + } + if !config.CachingMode.IsValid() { + return nil, fmt.Errorf("unexpected value for disk 'cache' option: %s (valid values: automatic, cached, uncached)", config.CachingMode) + } + if !config.SynchronizationMode.IsValid() { + return nil, fmt.Errorf("unexpected value for disk 'sync' option: %s (valid values: full, fsync, none)", config.SynchronizationMode) + } + if err := config.validate(); err != nil { + return nil, err + } value := fmt.Sprintf("%s,path=%s", config.DevName, config.ImagePath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/virtio.go` around lines 1002 - 1025, The ToCmdLine method on DiskStorageConfig currently emits cache/sync/type combinations without validating them; change DiskStorageConfig.ToCmdLine to validate Type, CachingMode, and SynchronizationMode before building the command-line string (the same checks that FromOptions applies) and return an error if any value is invalid so callers who set the public fields directly cannot produce unsupported combinations; locate the checks around DiskStorageConfig.Type, .CachingMode, and .SynchronizationMode in ToCmdLine and either call the existing validation helpers used by FromOptions or inline equivalent validation logic, then only append ",cache=..." and ",sync=..." when those values pass validation.pkg/vf/virtio.go (1)
539-576:⚠️ Potential issue | 🟠 MajorReject invalid disk modes here instead of silently rewriting them.
FromOptions()is not in this call chain. ADiskStorageConfigpopulated via JSON or direct struct assignment can still reachtoVz()withtype=dev,cache=*ortype=dev,synchronizationMode=fsync, and the current mapping either ignores the cache or downgradesfsynctofull. Please add a last-line validation here, or make the helpers fail on unsupported non-empty values, so invalid configs are rejected instead of being applied with different semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/vf/virtio.go` around lines 539 - 576, DiskStorageConfig.toVz() currently silently remaps unsupported caching/synchronization values (via toVzCachingMode, toVzSyncMode, toVzBlockDeviceSyncMode) which lets invalid configs slip through; change it to explicitly reject unsupported non-empty values by returning an error. Update DiskStorageConfig.toVz() (both the DiskBackendImage/DiskBackendDefault branch and the DiskBackendBlockDevice branch) to validate conf.CachingMode and conf.SynchronizationMode: either make the helper functions (toVzCachingMode/toVzSyncMode/toVzBlockDeviceSyncMode) return an error when given an unsupported non-empty string, or after calling them check whether the input was non-empty and the mapping indicates an unsupported/Default fallback and return fmt.Errorf("unsupported caching/synchronization mode: %q") instead of continuing to create the vz attachment; do not rely on FromOptions() for this validation.
🧹 Nitpick comments (1)
pkg/config/json_test.go (1)
284-285: Keep the new JSON fields under stability coverage.Adding
CachingModeandSynchronizationModetoskipFieldsmeans future renames/removals ofcachingModeandsynchronizationModewill no longer fail this backwards-compat test. Prefer seeding valid enum values in eachnewObjectFunc(or teachingfillStructabout these enum types) and updating the expected JSON instead.Also applies to: 294-295, 304-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/json_test.go` around lines 284 - 285, The test currently masks CachingMode and SynchronizationMode by adding them to skipFields (in json_test.go), which hides breakages; instead, remove "CachingMode" and "SynchronizationMode" from skipFields and seed valid enum values for these fields in each newObjectFunc (or extend fillStruct to set known enum values for cachingMode and synchronizationMode) so the expectedJSON strings are updated to include the concrete values; update the expectedJSON variables accordingly (also apply the same change to the other occurrences noted around the other test cases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/usage.md`:
- Around line 239-252: The three fenced code blocks showing example --device
commands (e.g., "--device virtio-blk,path=/dev/disk2,type=dev,sync=none",
"--device virtio-blk,path=/Users/virtuser/vfkit.img,cache=cached,sync=none", and
"--device virtio-blk,path=/Users/virtuser/vfkit.img,cache=uncached,sync=full")
should include a language tag to satisfy markdownlint MD040; update each opening
triple-backtick to "```bash" in the doc/usage.md examples at the shown ranges
and also add "```bash" to the other affected blocks referenced (around lines
295-302 and 337-349) so all three fenced blocks are explicitly marked as bash.
- Around line 213-225: The docs claim image-backed disks default to
CachingModeAutomatic but the runtime maps an omitted cache option to cached;
update pkg/vf/virtio.go so the cache-parsing/initialization code (the function
that maps disk options to CachingMode values — where omitted values are
currently set to CachingModeCached) instead defaults to CachingModeAutomatic for
type=image, leaving block-device defaults unchanged; ensure the symbol
CachingModeAutomatic is used for the omitted-case path and add/update any tests
or validation that assert the new default behavior.
In `@pkg/config/virtio.go`:
- Around line 648-656: The early return for block devices (check against
DiskBackendBlockDevice) skips required-path validation and allows
virtio-blk,type=dev to pass without a path; before returning for block devices,
ensure the mandatory path is validated (e.g., reuse or call whatever
path/required-fields check used elsewhere for Disk config, or explicitly
validate dev.Path) so the missing path is rejected early; keep the qcow2 probe
skipped but perform the path validation prior to the return in the same block
where dev.DiskStorageConfig.validate() and DiskBackendBlockDevice are checked.
---
Outside diff comments:
In `@pkg/config/virtio.go`:
- Around line 1002-1025: The ToCmdLine method on DiskStorageConfig currently
emits cache/sync/type combinations without validating them; change
DiskStorageConfig.ToCmdLine to validate Type, CachingMode, and
SynchronizationMode before building the command-line string (the same checks
that FromOptions applies) and return an error if any value is invalid so callers
who set the public fields directly cannot produce unsupported combinations;
locate the checks around DiskStorageConfig.Type, .CachingMode, and
.SynchronizationMode in ToCmdLine and either call the existing validation
helpers used by FromOptions or inline equivalent validation logic, then only
append ",cache=..." and ",sync=..." when those values pass validation.
In `@pkg/vf/virtio.go`:
- Around line 539-576: DiskStorageConfig.toVz() currently silently remaps
unsupported caching/synchronization values (via toVzCachingMode, toVzSyncMode,
toVzBlockDeviceSyncMode) which lets invalid configs slip through; change it to
explicitly reject unsupported non-empty values by returning an error. Update
DiskStorageConfig.toVz() (both the DiskBackendImage/DiskBackendDefault branch
and the DiskBackendBlockDevice branch) to validate conf.CachingMode and
conf.SynchronizationMode: either make the helper functions
(toVzCachingMode/toVzSyncMode/toVzBlockDeviceSyncMode) return an error when
given an unsupported non-empty string, or after calling them check whether the
input was non-empty and the mapping indicates an unsupported/Default fallback
and return fmt.Errorf("unsupported caching/synchronization mode: %q") instead of
continuing to create the vz attachment; do not rely on FromOptions() for this
validation.
---
Nitpick comments:
In `@pkg/config/json_test.go`:
- Around line 284-285: The test currently masks CachingMode and
SynchronizationMode by adding them to skipFields (in json_test.go), which hides
breakages; instead, remove "CachingMode" and "SynchronizationMode" from
skipFields and seed valid enum values for these fields in each newObjectFunc (or
extend fillStruct to set known enum values for cachingMode and
synchronizationMode) so the expectedJSON strings are updated to include the
concrete values; update the expectedJSON variables accordingly (also apply the
same change to the other occurrences noted around the other test cases).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a60fac6-5306-4a85-b473-f33306cde2b3
📒 Files selected for processing (5)
doc/usage.mdpkg/config/json_test.gopkg/config/virtio.gopkg/config/virtio_test.gopkg/vf/virtio.go
Comment on lines
+213
to
+225
| - `cache`: disk image caching mode (only for `type=image`, not supported for block devices). Valid values: | ||
| - `automatic` (default): allows the virtualization framework to automatically determine whether to enable data caching | ||
| - `cached`: enables data caching | ||
| - `uncached`: disables data caching | ||
| - `sync`: synchronization mode. Valid values differ by backing type: | ||
| - For disk images (`type=image`): | ||
| - `full`: synchronizes data to the permanent storage | ||
| - `fsync` (default): synchronizes data using the system's best-effort synchronization mode | ||
| - `none`: disables data synchronization | ||
| - For block devices (`type=dev`): | ||
| - `full` (default): synchronizes data to the permanent storage | ||
| - `none`: disables data synchronization | ||
| - Note: `fsync` is not supported for block devices |
There was a problem hiding this comment.
The documented cache default does not match the current implementation.
These sections say image-backed disks default to automatic, but pkg/vf/virtio.go currently maps an omitted cache mode to cached. Either document cached as the current backward-compatible default, or initialize CachingModeAutomatic when the option is omitted so the runtime behavior matches the docs.
Also applies to: 274-286, 316-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/usage.md` around lines 213 - 225, The docs claim image-backed disks
default to CachingModeAutomatic but the runtime maps an omitted cache option to
cached; update pkg/vf/virtio.go so the cache-parsing/initialization code (the
function that maps disk options to CachingMode values — where omitted values are
currently set to CachingModeCached) instead defaults to CachingModeAutomatic for
type=image, leaving block-device defaults unchanged; ensure the symbol
CachingModeAutomatic is used for the omitted-case path and add/update any tests
or validation that assert the new default behavior.
Comment on lines
+239
to
+252
| Block device with sync mode disabled for maximum performance: | ||
| ``` | ||
| --device virtio-blk,path=/dev/disk2,type=dev,sync=none | ||
| ``` | ||
|
|
||
| For ephemeral VMs where data persistence is not critical (maximize performance): | ||
| ``` | ||
| --device virtio-blk,path=/Users/virtuser/vfkit.img,cache=cached,sync=none | ||
| ``` | ||
|
|
||
| For database or critical workloads (maximize data safety): | ||
| ``` | ||
| --device virtio-blk,path=/Users/virtuser/vfkit.img,cache=uncached,sync=full | ||
| ``` |
There was a problem hiding this comment.
Add a language to the new fenced examples.
These blocks are the ones currently tripping markdownlint MD040. Marking them as bash will keep the docs lint-clean.
Also applies to: 295-302, 337-349
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 240-240: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 245-245: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 250-250: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/usage.md` around lines 239 - 252, The three fenced code blocks showing
example --device commands (e.g., "--device
virtio-blk,path=/dev/disk2,type=dev,sync=none", "--device
virtio-blk,path=/Users/virtuser/vfkit.img,cache=cached,sync=none", and "--device
virtio-blk,path=/Users/virtuser/vfkit.img,cache=uncached,sync=full") should
include a language tag to satisfy markdownlint MD040; update each opening
triple-backtick to "```bash" in the doc/usage.md examples at the shown ranges
and also add "```bash" to the other affected blocks referenced (around lines
295-302 and 337-349) so all three fenced blocks are explicitly marked as bash.
Comment on lines
+648
to
+656
| // First validate the disk storage config (cache/sync mode constraints) | ||
| if err := dev.DiskStorageConfig.validate(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Skip qcow2 check for block devices (they can't be qcow2 format) | ||
| if dev.Type == DiskBackendBlockDevice { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Keep the block-device fast path from skipping required-path validation.
This early return means virtio-blk,type=dev now parses successfully even when no path was provided, and the error is deferred until much later. Skipping the qcow2 probe for block devices makes sense, but the mandatory path check should still happen before returning.
One small way to preserve the old validation behavior
func (dev *VirtioBlk) validate() error {
// First validate the disk storage config (cache/sync mode constraints)
if err := dev.DiskStorageConfig.validate(); err != nil {
return err
}
+ if dev.ImagePath == "" {
+ return fmt.Errorf("virtio-blk devices need the path to a disk image or block device")
+ }
// Skip qcow2 check for block devices (they can't be qcow2 format)
if dev.Type == DiskBackendBlockDevice {
return nil
}📝 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.
Suggested change
| // First validate the disk storage config (cache/sync mode constraints) | |
| if err := dev.DiskStorageConfig.validate(); err != nil { | |
| return err | |
| } | |
| // Skip qcow2 check for block devices (they can't be qcow2 format) | |
| if dev.Type == DiskBackendBlockDevice { | |
| return nil | |
| } | |
| // First validate the disk storage config (cache/sync mode constraints) | |
| if err := dev.DiskStorageConfig.validate(); err != nil { | |
| return err | |
| } | |
| if dev.ImagePath == "" { | |
| return fmt.Errorf("virtio-blk devices need the path to a disk image or block device") | |
| } | |
| // Skip qcow2 check for block devices (they can't be qcow2 format) | |
| if dev.Type == DiskBackendBlockDevice { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/virtio.go` around lines 648 - 656, The early return for block
devices (check against DiskBackendBlockDevice) skips required-path validation
and allows virtio-blk,type=dev to pass without a path; before returning for
block devices, ensure the mandatory path is validated (e.g., reuse or call
whatever path/required-fields check used elsewhere for Disk config, or
explicitly validate dev.Path) so the missing path is rejected early; keep the
qcow2 probe skipped but perform the path validation prior to the return in the
same block where dev.DiskStorageConfig.validate() and DiskBackendBlockDevice are
checked.
Add user-configurable caching and synchronization modes for virtio-blk devices to allow fine-tuning performance vs data safety trade-offs. New options: - cache: automatic/cached/uncached (controls data caching) - sync: full/fsync/none (controls disk synchronization) This allows users to optimize for different workloads (ephemeral VMs, databases, etc.) without relying on hardcoded framework defaults. Fixes crc-org#383 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Both NVMExpressController and USBMassStorage already inherit these options through DiskStorageConfig, so only tests and documentation were needed. This provides consistent cache and sync mode configuration across all disk-based storage devices (virtio-blk, NVMe, USB mass storage). Related to crc-org#383 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Block devices (type=dev) use a different code path that doesn't support disk image caching and synchronization modes. Add validation in DiskStorageConfig to reject these options when type=dev is specified. This prevents invalid configurations and provides clear error messages to users who attempt to use these options with block devices. Related to crc-org#383 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Extends sync mode configuration to block devices (type=dev) for virtio-blk, NVMe, and USB mass storage devices. Block devices support 'full' and 'none' sync modes but not 'fsync' (which is only available for disk images), as the underlying VZ framework uses a different API for block device attachments. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
cfergeau
force-pushed
the
cache-sync-options
branch
from
91d427b to
22cd5ad
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/virtio.go (1)
1002-1025:⚠️ Potential issue | 🟠 MajorRun
DiskStorageConfig.validate()before emitting the CLI.Right now only
FromOptions()applies the newtype=devcache/sync rules. Callers that buildVirtioBlk,NVMExpressController, orUSBMassStoragestructs directly can still serialize combinations that the parser itself rejects.Minimal fix
func (config *DiskStorageConfig) ToCmdLine() ([]string, error) { + if err := config.validate(); err != nil { + return nil, err + } if config.ImagePath == "" { return nil, fmt.Errorf("%s devices need the path to a disk image", config.DevName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/virtio.go` around lines 1002 - 1025, The ToCmdLine method on DiskStorageConfig should call DiskStorageConfig.validate() and return its error before building the CLI string so invalid type/cache/sync combinations are rejected consistently; update the ToCmdLine implementation (the function named ToCmdLine on DiskStorageConfig) to invoke validate() at the start (same validation used by FromOptions()) and propagate any error, ensuring callers that construct VirtioBlk, NVMExpressController, or USBMassStorage structs directly cannot serialize invalid configs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/usage.md`:
- Around line 272-273: The NVMe and USB device description text currently states
devices are backed by image files only, which contradicts the new `path`/`type`
entries that document `type=dev`; update the prose in the NVMe and USB sections
to mention that backends can be either disk image files or host block devices
and include a short note about `type=dev` (and potential root privileges) to
match the `path`/`type` bullets and avoid the contradiction (refer to the `path`
and `type` fields and the `type=dev` example when editing those section
descriptions).
In `@pkg/config/virtio.go`:
- Around line 1044-1055: The parsing for disk options "cache" and "sync"
currently allows empty strings because DiskImageCachingMode/
DiskImageSynchronizationMode .IsValid() treats "" as valid; update the switch
arms that handle option.value (the "cache" and "sync" cases) to explicitly
reject an empty option.value (e.g., if option.value == "" return an error
stating empty value is not allowed for disk 'cache' or 'sync') before converting
with DiskImageCachingMode/DiskImageSynchronizationMode and assigning to
config.CachingMode/config.SynchronizationMode so empty assignments like "cache="
or "sync=" fail fast.
In `@pkg/vf/virtio.go`:
- Around line 498-537: The helpers toVzCachingMode, toVzSyncMode and
toVzBlockDeviceSyncMode currently map any unknown mode to a benign VZ default;
change them so the default branch only returns the backward-compatible VZ value
when the incoming config value is the empty/zero value (mode == ""), and do not
silently translate other invalid/non-empty values. Instead, add validation in
DiskStorageConfig.toVz() to check the input enum strings before calling these
helpers and return an error for unknown values (or propagate validation failure)
so invalid values like "fsync" for block devices are rejected rather than
collapsed to a valid VZ enum.
---
Outside diff comments:
In `@pkg/config/virtio.go`:
- Around line 1002-1025: The ToCmdLine method on DiskStorageConfig should call
DiskStorageConfig.validate() and return its error before building the CLI string
so invalid type/cache/sync combinations are rejected consistently; update the
ToCmdLine implementation (the function named ToCmdLine on DiskStorageConfig) to
invoke validate() at the start (same validation used by FromOptions()) and
propagate any error, ensuring callers that construct VirtioBlk,
NVMExpressController, or USBMassStorage structs directly cannot serialize
invalid configs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15f906db-3111-4aa0-99eb-4c14e1e76274
📒 Files selected for processing (5)
doc/usage.mdpkg/config/json_test.gopkg/config/virtio.gopkg/config/virtio_test.gopkg/vf/virtio.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/json_test.go
Comment on lines
+272
to
+273
| - `path`: the absolute path to the disk image file or block device. | ||
| - `type`: the backing type. Use `image` (default) for a disk image file, or `dev` to attach a host block device. Attaching a block device may require root privileges; use with care. |
There was a problem hiding this comment.
Update the NVMe and USB prose to mention block-device backends.
These new path/type entries document type=dev, but the Description text above both sections still says those devices are backed by image files only. The docs now contradict themselves on whether block devices are supported.
Also applies to: 313-314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/usage.md` around lines 272 - 273, The NVMe and USB device description
text currently states devices are backed by image files only, which contradicts
the new `path`/`type` entries that document `type=dev`; update the prose in the
NVMe and USB sections to mention that backends can be either disk image files or
host block devices and include a short note about `type=dev` (and potential root
privileges) to match the `path`/`type` bullets and avoid the contradiction
(refer to the `path` and `type` fields and the `type=dev` example when editing
those section descriptions).
Comment on lines
+1044
to
+1055
| case "cache": | ||
| mode := DiskImageCachingMode(option.value) | ||
| if !mode.IsValid() { | ||
| return fmt.Errorf("unexpected value for disk 'cache' option: %s (valid values: automatic, cached, uncached)", option.value) | ||
| } | ||
| config.CachingMode = mode | ||
| case "sync": | ||
| mode := DiskImageSynchronizationMode(option.value) | ||
| if !mode.IsValid() { | ||
| return fmt.Errorf("unexpected value for disk 'sync' option: %s (valid values: full, fsync, none)", option.value) | ||
| } | ||
| config.SynchronizationMode = mode |
There was a problem hiding this comment.
Reject explicit empty cache / sync values.
Because IsValid() accepts "", inputs like cache= and sync= are parsed as if the option was omitted. An empty shell variable can therefore silently change the storage mode instead of failing fast.
Minimal fix
case "cache":
+ if option.value == "" {
+ return fmt.Errorf("disk 'cache' option requires a value")
+ }
mode := DiskImageCachingMode(option.value)
if !mode.IsValid() {
return fmt.Errorf("unexpected value for disk 'cache' option: %s (valid values: automatic, cached, uncached)", option.value)
}
config.CachingMode = mode
case "sync":
+ if option.value == "" {
+ return fmt.Errorf("disk 'sync' option requires a value")
+ }
mode := DiskImageSynchronizationMode(option.value)
if !mode.IsValid() {
return fmt.Errorf("unexpected value for disk 'sync' option: %s (valid values: full, fsync, none)", option.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.
Suggested change
| case "cache": | |
| mode := DiskImageCachingMode(option.value) | |
| if !mode.IsValid() { | |
| return fmt.Errorf("unexpected value for disk 'cache' option: %s (valid values: automatic, cached, uncached)", option.value) | |
| } | |
| config.CachingMode = mode | |
| case "sync": | |
| mode := DiskImageSynchronizationMode(option.value) | |
| if !mode.IsValid() { | |
| return fmt.Errorf("unexpected value for disk 'sync' option: %s (valid values: full, fsync, none)", option.value) | |
| } | |
| config.SynchronizationMode = mode | |
| case "cache": | |
| if option.value == "" { | |
| return fmt.Errorf("disk 'cache' option requires a value") | |
| } | |
| mode := DiskImageCachingMode(option.value) | |
| if !mode.IsValid() { | |
| return fmt.Errorf("unexpected value for disk 'cache' option: %s (valid values: automatic, cached, uncached)", option.value) | |
| } | |
| config.CachingMode = mode | |
| case "sync": | |
| if option.value == "" { | |
| return fmt.Errorf("disk 'sync' option requires a value") | |
| } | |
| mode := DiskImageSynchronizationMode(option.value) | |
| if !mode.IsValid() { | |
| return fmt.Errorf("unexpected value for disk 'sync' option: %s (valid values: full, fsync, none)", option.value) | |
| } | |
| config.SynchronizationMode = mode |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/virtio.go` around lines 1044 - 1055, The parsing for disk options
"cache" and "sync" currently allows empty strings because DiskImageCachingMode/
DiskImageSynchronizationMode .IsValid() treats "" as valid; update the switch
arms that handle option.value (the "cache" and "sync" cases) to explicitly
reject an empty option.value (e.g., if option.value == "" return an error
stating empty value is not allowed for disk 'cache' or 'sync') before converting
with DiskImageCachingMode/DiskImageSynchronizationMode and assigning to
config.CachingMode/config.SynchronizationMode so empty assignments like "cache="
or "sync=" fail fast.
Comment on lines
+498
to
+537
| func toVzCachingMode(mode config.DiskImageCachingMode) vz.DiskImageCachingMode { | ||
| switch mode { | ||
| case config.CachingModeAutomatic: | ||
| return vz.DiskImageCachingModeAutomatic | ||
| case config.CachingModeCached: | ||
| return vz.DiskImageCachingModeCached | ||
| case config.CachingModeUncached: | ||
| return vz.DiskImageCachingModeUncached | ||
| default: | ||
| // Default to cached for backward compatibility | ||
| return vz.DiskImageCachingModeCached | ||
| } | ||
| } | ||
|
|
||
| func toVzSyncMode(mode config.DiskImageSynchronizationMode) vz.DiskImageSynchronizationMode { | ||
| switch mode { | ||
| case config.SyncModeFull: | ||
| return vz.DiskImageSynchronizationModeFull | ||
| case config.SyncModeFsync: | ||
| return vz.DiskImageSynchronizationModeFsync | ||
| case config.SyncModeNone: | ||
| return vz.DiskImageSynchronizationModeNone | ||
| default: | ||
| // Default to fsync for backward compatibility | ||
| return vz.DiskImageSynchronizationModeFsync | ||
| } | ||
| } | ||
|
|
||
| func toVzBlockDeviceSyncMode(mode config.DiskImageSynchronizationMode) vz.DiskSynchronizationMode { | ||
| switch mode { | ||
| case config.SyncModeFull: | ||
| return vz.DiskSynchronizationModeFull | ||
| case config.SyncModeNone: | ||
| return vz.DiskSynchronizationModeNone | ||
| default: | ||
| // Default to full for backward compatibility | ||
| // Note: fsync is not supported for block devices and should be caught by validation | ||
| return vz.DiskSynchronizationModeFull | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't collapse invalid disk modes to real VZ modes.
These default branches treat every unknown non-empty value as if the mode was merely unset. That lets JSON/direct-struct callers bypass FromOptions() and launch with cached / fsync / full instead of failing for an invalid enum or for type=dev,sync=fsync.
Please reserve the fallback for "" only, or validate in DiskStorageConfig.toVz() before calling these helpers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/vf/virtio.go` around lines 498 - 537, The helpers toVzCachingMode,
toVzSyncMode and toVzBlockDeviceSyncMode currently map any unknown mode to a
benign VZ default; change them so the default branch only returns the
backward-compatible VZ value when the incoming config value is the empty/zero
value (mode == ""), and do not silently translate other invalid/non-empty
values. Instead, add validation in DiskStorageConfig.toVz() to check the input
enum strings before calling these helpers and return an error for unknown values
(or propagate validation failure) so invalid values like "fsync" for block
devices are rejected rather than collapsed to a valid VZ enum.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds cache syncing options, both for file-backed virtio-blk
instances, and block-device backed virtio-blk devices.
This was mostly authored by Claude
Summary by CodeRabbit
New Features
cacheandsyncparameters for disk devices to control I/O caching and synchronization for image-backed and block-device-backed storage.Documentation
Validation
cachefor block-device-backed disks and rejects unsupported sync modes for block devices; acceptssync=full/sync=nonewhere applicable.Tests