Qcow2 Support

[Miles Hansell (milosilo-dev)]

The PR plans to add support for the Qcow2 image format, as referenced in issue #1534.

I plan to only add basic support to start with (no snapshotting, no encryption) so that it can be expanded in the future.

I have decided against using a library because, of the crates that exist for qcow2 support, the only one I could find that is actively maintained is qcow2-rs however it would require pulling in a lot of new dependencies and I am not sure weather it would be worth it. if anyone else can find a suitable library please let me know but for the moment I think it would be easier to just implement it myself.

This PR is now in a state where I believe it can be merged, while it does not have complete spec implementation, it is in a state where it can reliably read and write to QCOW2 images.

[Miles Hansell (milosilo-dev)]

@microsoft-github-policy-service agree

[Copilot]

Pull request overview

Adds an initial qcow2 “disk layer” crate and wires it into the OpenVMM resource resolver pipeline so qcow2 files can be selected as a disk type (non-Windows path) and resolved into a LayeredDisk layer.

Changes:

• Introduces new disklayer_qcow2 crate with header parsing, chain opening helpers, and a resource resolver.
• Adds a Qcow2DiskLayerHandle resource type and registers the qcow2 resolver in openvmm_resources.
• Extends openvmm_helpers::open_disk_type() to recognize .qcow2 on non-Windows hosts.

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
vm/devices/storage/disklayer_qcow2/src/resolver.rs	Adds a resolver that reads qcow2 header and constructs a qcow2 layer.
vm/devices/storage/disklayer_qcow2/src/lib.rs	Introduces Qcow2Layer implementing LayerIo (currently placeholder behavior).
vm/devices/storage/disklayer_qcow2/src/header.rs	Implements qcow2 header parsing and validation (needs fixes for compilation/panic-safety).
vm/devices/storage/disklayer_qcow2/src/chain.rs	Adds helpers to open qcow2 files as layered-disk resources.
vm/devices/storage/disklayer_qcow2/Cargo.toml	New crate manifest for qcow2 layer integration.
vm/devices/storage/disk_backend_resources/src/layer.rs	Adds Qcow2DiskLayerHandle resource handle type.
openvmm/openvmm_resources/src/lib.rs	Registers qcow2 disk layer resolver.
openvmm/openvmm_resources/Cargo.toml	Adds dependency on disklayer_qcow2.
openvmm/openvmm_helpers/src/disk.rs	Adds .qcow2 detection/dispatch in open_disk_type().
openvmm/openvmm_helpers/Cargo.toml	Adds dependency on disklayer_qcow2.
Cargo.toml	Adds workspace dependency entry for disklayer_qcow2.
Cargo.lock	Records the new crate in the lockfile and dependency graph.

Suppressed comments (2)

vm/devices/storage/disklayer_qcow2/src/header.rs:73

• Avoid unwrap() when parsing qcow2 header bytes (untrusted input). Use direct indexing instead so malformed inputs can’t trigger a panic path.

    Ok(u32::from_be_bytes(int_bytes.try_into().unwrap()))

vm/devices/storage/disklayer_qcow2/src/header.rs:81

• Avoid unwrap() when parsing qcow2 header bytes (untrusted input). Use direct indexing instead so malformed inputs can’t trigger a panic path.

    Ok(u64::from_be_bytes(int_bytes.try_into().unwrap()))

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

Previously missed (6) — in code that hasn't changed since the last review.

vm/devices/storage/disklayer_qcow2/src/header.rs:193

• Qcow2Layer hard-codes a 512-byte sector size and derives sector_count by integer-dividing size_bytes by 512. If size_bytes is not a multiple of 512, this will silently truncate the exposed disk size. Consider validating 512-byte alignment when parsing the header (or rounding up) to keep LayerIo invariants consistent.

        );
        Ok(value)
    }

    fn read_disk_size(header: &mut &[u8]) -> anyhow::Result<u64> {

vm/devices/storage/disklayer_qcow2/src/chain.rs:46

• open_qcow2_chain_explicit always opens layers with write access (.write(true)), and the handle is currently always constructed with read_only: false. This makes OpenDiskOptions::read_only ineffective for qcow2 and diverges from the vhdx chain convention of opening parents read-only.

    anyhow::ensure!(!paths.is_empty(), "qcow2 chain must have at least one file");

    let mut layers = Vec::new();
    for (i, path) in paths.iter().enumerate() {
        let file = std::fs::OpenOptions::new()

vm/devices/storage/disklayer_qcow2/src/resolver.rs:45

• This resolver performs blocking seek/read operations on a std::fs::File directly inside an async resolve() implementation. Other disk resolvers (e.g. disklayer_vhdx) offload file I/O using blocking::unblock to avoid stalling the executor.

        resource.file.seek(std::io::SeekFrom::Start(0))?;
        let header = Qcow2Header::from_file(&mut resource.file)?;
        resource.file.seek(std::io::SeekFrom::Start(0))?;
        Ok(ResolvedDiskLayer::new(Qcow2Layer::new(

vm/devices/storage/disklayer_qcow2/src/header.rs:176

• The qcow2 spec allows cluster_bits up to 21 (2 MiB clusters). Limiting this to 16 will reject valid qcow2 images, and the surrounding comment says the checks are spec-based rather than implementation-based.

        Ok(file_size)
    }

    fn read_cluster_bits(header: &mut &[u8]) -> anyhow::Result<u32> {
        let cluster_bits = read_be_u32(header)?;

vm/devices/storage/disklayer_qcow2/src/header.rs:112

• crypt_method is parsed but never validated. Since encryption isn't supported yet (per PR description), it would be safer to fail fast when crypt_method != 0 so we don't proceed with an image we can't interpret correctly.

This issue also appears on line 115 of the same file.

            cluster_bits: {
                cluster_bits = Self::read_cluster_bits(header)?;
                cluster_bits
            },
            size_bytes: Self::read_disk_size(header)?,

vm/devices/storage/disklayer_qcow2/Cargo.toml:22

• guestmem/guid/thiserror/vhdx are currently listed as dependencies but don't appear to be used anywhere in this new crate yet. In this repo, cargo xtask fmt --fix typically strips unused deps, so leaving them in will likely cause churn or CI formatting noise.

guestmem.workspace = true
guid.workspace = true

vm/devices/storage/disklayer_qcow2/src/header.rs:115

• snapshots_offset is documented as needing cluster alignment, but it's currently read without using read_u64_cluster_aligned(). Accepting an unaligned snapshots_offset could lead to incorrect parsing later.

            l1_table_offset: Self::read_u64_cluster_aligned(header, cluster_bits)?,

[Copilot]

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

vm/devices/storage/disklayer_qcow2/src/resolver.rs:28

• The doc comment above the resolver still describes manually reading the size field at a fixed offset, but the implementation now parses a full Qcow2Header. This is misleading and should be updated to match the current behavior.

/// Read the virtual disk size (in bytes) from the qcow2 header.
///
/// The header's `size` field is a big-endian u64 at offset 24.

vm/devices/storage/disklayer_qcow2/src/resolver.rs:49

• resolve returns a Qcow2Layer even though the layer’s read/write/unmap methods currently always error. That means qcow2 disks may appear to open successfully but then fail later during I/O with a generic io error. Until the layer implements at least basic reads, it would be better to fail during resolution with a clear error.

        resource.file.seek(std::io::SeekFrom::Start(0))?;
        let header = Qcow2Header::from_file(&mut resource.file)?;
        resource.file.seek(std::io::SeekFrom::Start(0))?;
        Ok(ResolvedDiskLayer::new(Qcow2Layer::new(
            resource.file,
            header,
            read_only,
        )))

vm/devices/storage/disklayer_qcow2/src/header.rs:182

• The qcow2 spec allows cluster sizes up to 2 MiB (cluster_bits up to 21). Restricting this to 16 will reject valid qcow2 images unnecessarily.

    fn read_cluster_bits(header: &mut &[u8]) -> anyhow::Result<u32> {
        let cluster_bits = read_be_u32(header)?;
        anyhow::ensure!(
            (9..=16).contains(&cluster_bits),
            "Cluster bits must be between 9 and 16 for qcow2"
        );

vm/devices/storage/disklayer_qcow2/Cargo.toml:28

• disklayer_qcow2 declares several dependencies (and dev-dependencies) that are not used anywhere in the crate right now. In this repo, cargo xtask fmt --fix typically removes unused Cargo.toml entries, so keeping these will likely cause churn or CI failures once formatting/lint checks run.

blocking.workspace = true
disk_backend.workspace = true
disk_backend_resources.workspace = true
disk_layered.workspace = true
guestmem.workspace = true
guid.workspace = true
inspect.workspace = true
scsi_buffers.workspace = true
thiserror.workspace = true
vhdx.workspace = true
vm_resource.workspace = true

[dev-dependencies]
pal_async.workspace = true
storage_tests.workspace = true
tempfile.workspace = true

vm/devices/storage/disklayer_qcow2/src/lib.rs:106

• This placeholder read implementation doesn’t mutate marker, so the mut marker binding will trigger an unused_mut warning under typical lint settings. Using underscored parameter names avoids both unused-variable and unused-mutable warnings.

    async fn read(
        &self,
        buffers: &RequestBuffers<'_>,
        sector: u64,
        mut marker: SectorMarker<'_>,
    ) -> Result<(), DiskError> {
        let _ = (buffers, sector, marker);

[Steven Malis (smalis-msft)]

Thanks for your contribution! I'm not super familiar with the qcow2 format, but I'll take a look!

[Steven Malis (smalis-msft)]

Overall this definitely looks like it's going in the right direction! Left some feedback to hopefully make things cleaner, but it looks like a good start.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.

[Copilot]

🟡 Changes recommended

There are correctness issues in L2 entry decoding/write-back and missing safety checks before overwriting non-copied clusters, which can corrupt qcow2 metadata or shared-cluster images.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

vm/devices/storage/disklayer_qcow2/src/table.rs:214

• write_l2_table unconditionally ORs OFLAG_COPIED into every non-zero entry (let mut r = ... | OFLAG_COPIED;). This forces the COPIED bit on even when entry.copied is false, which can corrupt refcount semantics and breaks future COW logic.

        let raw = if entry.cluster_offset == 0 {
            0
        } else {
            let mut r = (entry.cluster_offset & OFFSET_MASK) | OFLAG_COPIED;
            if entry.compressed {
                r |= 1 << 62;
            }
            if entry.copied {
                r |= OFLAG_COPIED;
            }
            if entry.reads_as_zeros {
                r |= 1;
            }
            r
        };

vm/devices/storage/disklayer_qcow2/src/lib.rs:458

• The write path overwrites existing allocated clusters even when the L2 entry is not marked copied (refcount != 1). Without snapshot/backing-file support, the safest behavior is to reject writes to non-copied clusters (or implement COW); otherwise a crafted/edge-case image with shared clusters can be corrupted by in-place writes.

            let l2_entry = &l2_table[addr.l2_index as usize];
            if l2_entry.compressed {
                return Err(DiskError::InvalidInput);
            }

            let needs_allocation = l2_entry.cluster_offset == 0 || l2_entry.reads_as_zeros;
            let data_cluster_offset = if needs_allocation {
                let cluster_size = cluster_size as u64;
                let new_cluster = allocate_cluster(file.clone(), cluster_size).await?;
                zero_cluster(file.clone(), new_cluster, cluster_size as usize).await?;
                state
                    .refcounts
                    .increment_cluster(&file, new_cluster / cluster_size)
                    .await?;
                new_cluster
            } else {
                l2_entry.cluster_offset
            };

• Files reviewed: 15/16 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The qcow2 L2 table writeback currently encodes flags incorrectly (notably COPIED), which can corrupt metadata, and there are additional correctness/performance/doc gaps to address.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

vm/devices/storage/disklayer_qcow2/src/table.rs:212

• write_l2_table always sets the COPIED bit for any non-zero cluster_offset (| OFLAG_COPIED), even when entry.copied is false, and it also drops the reads_as_zeros flag when cluster_offset == 0. This will rewrite on-disk L2 entries incorrectly (e.g., converting shared clusters into COPIED).

        let raw = if entry.cluster_offset == 0 {
            0
        } else {
            let mut r = (entry.cluster_offset & OFFSET_MASK) | OFLAG_COPIED;
            if entry.compressed {

openvmm/openvmm_helpers/src/disk.rs:106

• CLI documentation currently lists flat/.vhd/.vhdx disk images as supported --disk file:<DISK> inputs (Guide/src/reference/openvmm/management/cli.md), but this code adds qcow2 support. Please update the Guide to mention .qcow2 (and any constraints like no snapshots/encryption yet) so users can discover it.

        Some("qcow2") => {
            ensure_no_direct(".qcow2")?;
            disklayer_qcow2::chain::open_qcow2_chain(path, read_only).await?
        }

• Files reviewed: 15/16 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are correctness and DoS-risk issues in qcow2 metadata serialization and header/refcount-table size bounding that should be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 15/16 changed files
• Comments generated: 5
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Confirmed correctness issues in qcow2 read/write/refcount logic (including overflow hazards and unsafe handling of “reads as zeros” entries) need to be addressed before merge.

Review details

Suppressed comments (6)

Previously missed (2) — in code that hasn't changed since the last review.

vm/devices/storage/disklayer_qcow2/src/lib.rs:260

• offset is computed before validate_range, so a very large sector can overflow (panic in debug / wrap in release) before the request is rejected. Compute and validate the request length first, then derive offset/end with checked arithmetic.

This issue also appears on line 382 of the same file.
vm/devices/storage/disklayer_qcow2/src/refcount.rs:157

• When allocating a missing refcount block, increment_cluster immediately extends the file, then may later discover that the newly allocated block’s own cluster index is beyond the refcount table coverage (the next loop iteration hits table_index >= self.entries.len()). That leaves the image permanently extended and returns an error. Pre-check that the next end-of-file allocation is still addressable before calling allocate_cluster.

vm/devices/storage/disklayer_qcow2/src/lib.rs:384

• Same overflow hazard as in read: offset/end are computed before/without checked arithmetic, so an out-of-range sector can overflow before validate_range rejects it. Use checked arithmetic after validating the request length.

        let offset = sector * SECTOR_SIZE as u64;
        let len = buffers.len();
        self.validate_range(sector, len)?;

vm/devices/storage/disklayer_qcow2/src/lib.rs:196

• validate_range uses sector + ... without checked_add, so an attacker-controlled sector can overflow and bypass the bounds check (or panic in debug). Use checked addition when computing end_sector.

        if !byte_len.is_multiple_of(SECTOR_SIZE as usize) {
            return Err(DiskError::InvalidInput);
        }
        let end_sector = sector + byte_len as u64 / SECTOR_SIZE as u64;
        if end_sector > self.sector_count {
            return Err(DiskError::IllegalBlock);

vm/devices/storage/disklayer_qcow2/src/lib.rs:453

• Writes treat an L2 entry with the "reads as all zeros" flag (bit 0) as already allocated if cluster_offset != 0, and will write into that host offset. Since the offset is explicitly ignored for reads, it should not be trusted as usable storage for writes; treat reads_as_zeros as needing allocation.

            let needs_allocation = l2_entry.cluster_offset == 0;
            let data_cluster_offset = if needs_allocation {
                let cluster_size = cluster_size as u64;

openvmm/openvmm_helpers/src/disk.rs:106

• This adds qcow2 support to --disk file:<DISK> resolution, but the CLI docs still list only flat/.vhd/.vhdx as supported disk images (Guide/src/reference/openvmm/management/cli.md around the --disk section). The docs should be updated to mention .qcow2 as a supported image type (and any platform limitations, if applicable).

        Some("qcow2") => {
            ensure_no_direct(".qcow2")?;
            disklayer_qcow2::chain::open_qcow2_chain(path, read_only).await?
        }

• Files reviewed: 15/16 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There’s at least one correctness concern in qcow2 table bit-masking plus significant lock/IO and L2-table allocation behavior that should be addressed to avoid rejecting valid images and to prevent serious runtime contention/memory overhead.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

vm/devices/storage/disklayer_qcow2/src/lib.rs:297

• Reading and decoding the entire L2 table for every cluster access is extremely allocation-heavy (and scales with cluster size), which can become a major CPU/memory bottleneck under guest-driven I/O. Only a single 8-byte L2 entry is needed for the current addr.l2_index; consider reading/decoding just that entry (and later adding caching if needed).

This issue also appears on line 427 of the same file.

vm/devices/storage/disklayer_qcow2/src/lib.rs:257

• The state mutex is held across multiple .await points in read() (e.g. while doing unblock(...).await), which serializes all reads and can also block writers behind slow I/O. Since read() only needs an immutable view of the L1 table, consider dropping the mutex guard before awaiting any file I/O.

        let file = self.file.clone();
        let state = self.state.lock().await;
        let l1_table = &state.l1_table;

openvmm/openvmm_helpers/src/disk.rs:106

• This adds user-visible qcow2 support (auto-detected by .qcow2 extension). The Guide currently documents converting qcow2 images to raw in at least one workflow (e.g. Guide/src/user_guide/openvmm/alpine.md). It would be good to update the relevant Guide docs/scripts to reflect that OpenVMM can now open qcow2 directly (or explicitly justify why conversion is still required in that guide).

        Some("qcow2") => {
            ensure_no_direct(".qcow2")?;
            disklayer_qcow2::chain::open_qcow2_chain(path, read_only).await?
        }

vm/devices/storage/disklayer_qcow2/src/lib.rs:444

• The write path currently reads and decodes the entire L2 table (and later writes the full table back) even though only one entry is being modified. This can make small random writes very expensive and allocation-heavy. Consider switching to read/modify/write of just the single 8-byte L2 entry (and only allocating/zeroing a full table when creating a brand-new L2 table), or introduce an L2 cache with targeted entry updates.

            // TODO: Read cache for Level 2 entries
            let mut l2_bytes = vec![0u8; l2_entries * 8];
            let f = file.clone();
            let l2_bytes = unblock(move || -> Result<Vec<u8>, std::io::Error> {
                let n = f.read_at(&mut l2_bytes, l2_offset)?;
                if n != l2_bytes.len() {
                    return Err(std::io::Error::new(
                        std::io::ErrorKind::UnexpectedEof,
                        "short read",
                    ));
                }
                Ok(l2_bytes)
            })
            .await
            .map_err(DiskError::Io)?;
            let mut l2_slice = l2_bytes.as_slice();
            let mut l2_table = read_l2_table(&mut l2_slice, l2_entries as u32)
                .map_err(|e| DiskError::Io(std::io::Error::other(e)))?;

• Files reviewed: 15/16 changed files
• Comments generated: 1
• Review effort level: Lite

[Miles Hansell (milosilo-dev)]

The last recommended change from copilot was incorrect, I believe that this should be ready to review. I will stop commiting now if someone wants to review my changes.

[github-actions]

1 Petri tests failed (0 unstable)

[Copilot]

🟡 Changes recommended

The current write path can corrupt qcow2 metadata/data by writing in-place to L2 tables/clusters even when copied indicates refcount sharing, and L2 serialization currently drops reads_as_zeros for zero-offset entries.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

vm/devices/storage/disklayer_qcow2/src/table.rs:210

• When serializing L2 entries, cluster_offset == 0 unconditionally writes a raw value of 0. This drops the reads_as_zeros flag for entries that intentionally force zero reads without a host allocation (i.e. raw == 1). After any write that updates the L2 table, such entries would silently become normal unallocated entries and could change read semantics through the layered stack.

        let raw = if entry.cluster_offset == 0 {
            0
        } else {

vm/devices/storage/disklayer_qcow2/src/lib.rs:449

• The write path ignores the L2 entry copied bit for data clusters. If copied is false (refcount may be >1), the code will overwrite the host cluster in-place, which can corrupt any other references to that cluster. Until proper COW + refcount updates are implemented, writes should reject non-copied allocated clusters (or implement COW).

            let l2_entry = &l2_table[addr.l2_index as usize];
            if l2_entry.compressed {
                return Err(DiskError::InvalidInput);
            }

openvmm/openvmm_helpers/src/disk.rs:106

• This adds a new user-facing disk type (--type qcow2 / "qcow2" in config). The Guide currently describes qcow2 images as something to convert to raw (e.g. Guide/src/user_guide/openvmm/alpine.md), so the documentation and/or scripts should be updated to reflect direct qcow2 support (or explicitly note any remaining limitations).

        Some("qcow2") => {
            ensure_no_direct(".qcow2")?;
            disklayer_qcow2::chain::open_qcow2_chain(path, read_only).await?
        }

• Files reviewed: 15/16 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

There are correctness issues in qcow2 metadata writeback and write safety (reads-as-zeros persistence and missing protection against overwriting non-copied/shared clusters).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

vm/devices/storage/disklayer_qcow2/src/chain.rs:53

• open_qcow2_chain_explicit is async but performs only blocking filesystem I/O and currently has no .await, which is likely to trigger clippy::unused_async under workspace lints and also risks blocking the async executor. Consider moving the file open into blocking::unblock so the function is genuinely async.

• Files reviewed: 15/16 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The qcow2 refcount-table sizing validation is currently stricter than necessary and can cause legitimate images to be rejected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

vm/devices/storage/disklayer_qcow2/src/lib.rs:256

• self.state.lock().await is held for the full duration of the read loop, including across multiple .await points for file I/O. This serializes all concurrent reads/writes through a single mutex and can become a significant throughput bottleneck (and increases tail latency) for large reads.

        let file = self.file.clone();
        let state = self.state.lock().await;
        let l1_table = &state.l1_table;

• Files reviewed: 15/16 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

2 Petri tests failed (0 unstable)