Skip to content

openvmm-img: add cross-platform disk image utility - #4370

Open
John Starks (jstarks) wants to merge 11 commits into
microsoft:mainfrom
jstarks:vhdxtool
Open

John Starks (jstarks) wants to merge 11 commits into
microsoft:mainfrom
jstarks:vhdxtool

Conversation

@jstarks

@jstarks John Starks (jstarks) commented Sep 2, 2026

Copy link
Copy Markdown
Member

Add openvmm-img, a cross-platform disk image utility initially supporting VHDX creation, inspection, allocation mapping, validation, log replay, and raw/VHDX conversion.

The immediate motivation is VHDX differencing-disk support, which is unavailable in qemu-img. This PR adds differencing-image creation and parent-chain validation, backed by typed VHDX creation options and parent-locator handling. Conversion of differencing images is not yet supported. The tool provides a foundation for exposing additional VHDX features as needed.

Although the initial focus is VHDX, the anticipated longer-term direction includes conversion between other image formats that OpenVMM supports or will eventually support. The generic name and format-neutral command structure accommodate that expansion.

Copilot AI lite review requested due to automatic review settings September 2, 2026 22:32
@github-actions github-actions Bot added the unsafe Related to unsafe code label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

⚠️ Unsafe Code Detected

This PR modifies files containing unsafe Rust code. Extra scrutiny is required during review.

For more on why we check whole files, instead of just diffs, check out the Rustonomicon

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Differencing-disk creation currently treats relative-path computation as mandatory (breaking common cross-root/drive cases) and there’s a misleading new test name that should be corrected to reflect actual assertions.

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

Pull request overview

Adds a new cross-platform vhdxtool CLI to create/inspect/map/convert/check/replay VHDX images, and extends the vhdx crate creation/open surface to represent disk allocation + differencing parent metadata with typed APIs (including parent-locator serialization/parsing) so invalid configurations are unrepresentable.

Changes:

  • Introduce vhdxtool (new crate) with commands for creating, inspecting, mapping, converting, validating parent chains, and replaying logs.
  • Refactor vhdx::CreateParams to use typed DiskType + VhdxParent, and add parent-locator building/parsing and open-error classification.
  • Update vhdx tests to the new creation API and validate locator behavior.
File summaries
File Description
vm/devices/storage/vhdxtool/src/util.rs Adds CLI helpers for size parsing/formatting and relative parent-path computation.
vm/devices/storage/vhdxtool/src/main.rs Implements the vhdxtool CLI and operational logic for create/info/map/convert/check/replay.
vm/devices/storage/vhdxtool/src/file.rs Provides a cross-platform AsyncFile implementation for positional I/O backing vhdxtool.
vm/devices/storage/vhdxtool/README.md Documents vhdxtool usage and expected behaviors (exit codes, parent locator notes, sparsity).
vm/devices/storage/vhdxtool/Cargo.toml Defines the new vhdxtool crate and dependencies.
vm/devices/storage/vhdx/tests/native_cross_validation.rs Updates cross-validation harness to create differencing disks via typed DiskType.
vm/devices/storage/vhdx/src/tests/trim_tests.rs Updates fixed-disk creation in trim tests to use DiskType::Fixed.
vm/devices/storage/vhdx/src/tests/mod.rs Updates integration tests to use DiskType::Differencing(...).
vm/devices/storage/vhdx/src/tests/io_tests.rs Updates I/O tests for differencing disks to use typed DiskType.
vm/devices/storage/vhdx/src/sector_bitmap.rs Updates sector-bitmap tests to use DiskType::Differencing(...).
vm/devices/storage/vhdx/src/open.rs Adds/updates tests around differencing opens and fully allocated behavior.
vm/devices/storage/vhdx/src/locator.rs Adds typed vhdx_parent() interpretation + makes locator construction size-safe.
vm/devices/storage/vhdx/src/lib.rs Re-exports new public types (DiskType, VhdxParent, OpenErrorKind, etc.).
vm/devices/storage/vhdx/src/error.rs Adds OpenErrorKind classification and new invalid-format reasons related to parent locators.
vm/devices/storage/vhdx/src/create.rs Implements typed disk creation + parent-locator metadata emission with size validation.
Cargo.toml Registers vhdxtool as a workspace member.
Cargo.lock Adds the new vhdxtool package entry and its resolved dependency set.
Review details
  • Files reviewed: 16/17 changed files
  • Comments generated: 2
  • Review effort level: Lite

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

Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdx/src/open.rs
Copilot AI review requested due to automatic review settings September 3, 2026 16:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

OpenError::kind() currently matches on self.0 by value (moving out of &self) and will not compile.

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

Review details

Suppressed comments (3)

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

vm/devices/storage/vhdx/src/locator.rs:235

  • The doc comment doesn’t explain why this now returns Option<Vec<u8>>. Since callers map None to ParentLocatorTooLarge, it would be helpful to document the failure conditions (oversize locator / overflow / too many entries).
    vm/devices/storage/vhdxtool/src/main.rs:764
  • Opening the newly created output file can fail, but this ? loses the path context in the error chain. Adding with_context here will make failures easier to diagnose (e.g., permission issues, file deleted between create and open).

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

vm/devices/storage/vhdxtool/src/main.rs:810

  • Same as above: this open is part of the conversion flow, but failing here won’t include the output path unless you add context.
                    let output_file = BlockingFile::open(output_path, false)?;
  • Files reviewed: 16/17 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread vm/devices/storage/vhdx/src/error.rs
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/openvmm-img/src/main.rs
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 22:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The current vhdxtool conversion path can silently produce incorrect output for differencing inputs, and there are a couple of concrete correctness/robustness issues to address before merging.

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

Review details

Suppressed comments (3)

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

vm/devices/storage/vhdxtool/src/file.rs:123

  • BlockingFile::file_size() performs a blocking metadata() syscall directly inside an async method. The other methods use blocking::unblock, so this stands out as an accidental blocking call that can stall the async executor thread.
    vm/devices/storage/vhdxtool/src/main.rs:359
  • On Windows, relative_path.ok() silently drops all errors from util::relative_path(...), not just the expected "no common root" case (e.g., canonicalize failures, permission errors). This can mask real problems and produce incomplete/incorrect parent locator metadata without any indication to the user.
    vm/devices/storage/vhdxtool/README.md:45
  • The README documents conversion examples, but it doesn't mention the current differencing-disk limitation (parent payload data is not incorporated). Without an explicit note, users may assume convert produces a fully materialized image when given a differencing VHDX.
  • Files reviewed: 16/17 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread vm/devices/storage/vhdxtool/src/main.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fixed images are not provisioned correctly, and force-mode path aliasing can destroy source or parent files.

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

Review details

Suppressed comments (3)

vm/devices/storage/vhdxtool/src/main.rs:370

  • The absolute locator has the same lossy-conversion problem: a Windows path containing an unpaired surrogate is recorded with replacement characters, leaving a child that cannot resolve its parent. Return an error when the canonical path is not valid Unicode.
            let absolute_path = absolute_parent.to_string_lossy();
            let absolute_path = if absolute_path.starts_with(r"\\?\") {
                absolute_path.into_owned()
            } else {
                format!(r"\\?\{}", absolute_path)

vm/devices/storage/vhdxtool/src/main.rs:378

  • With --force, the child output can alias its parent (the same path, a symlink, or a hard link). The parent is opened first, then this call truncates it and writes a differencing image whose locator refers to the destroyed parent. Reject output/parent file identity before truncating.
    let file = BlockingFile::create(&options.file, options.force)
        .with_context(|| format!("failed to create {}", options.file.display()))?;

vm/devices/storage/vhdxtool/src/main.rs:736

  • Before any output creation, reject cases where input and output identify the same file. With --force, every conversion branch opens the input and then truncates the aliased output, destroying the source before it can be copied; canonical spelling, symlinks, and hard links all need consideration.
    anyhow::ensure!(
        !matches!(disk_type, DiskType::Differencing),
        "convert output cannot be differencing without a parent"
    );
  • Files reviewed: 16/17 changed files
  • Comments generated: 7
  • Review effort level: Balanced

Comment thread vm/devices/storage/vhdx/src/create.rs
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdxtool/src/main.rs
Comment thread vm/devices/storage/vhdxtool/src/main.rs
Comment thread vm/devices/storage/vhdxtool/src/main.rs Outdated
Comment thread vm/devices/storage/vhdxtool/README.md Outdated
Comment thread vm/devices/storage/openvmm-img/src/main.rs
Copilot AI review requested due to automatic review settings September 10, 2026 17:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Fixed images are not actually allocated, and same-file operations can destructively truncate source or parent images.

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

Review details

Suppressed comments (6)

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

vm/devices/storage/vhdx/src/create.rs:60

  • This constructor permits a differencing parent with no locator path. create then serializes only parent_linkage, producing a parent locator that neither the repository chain resolver nor vhdxtool check can resolve. Require at least one relative, absolute Win32, or volume path before a VhdxParent can be used for creation.

vm/devices/storage/vhdxtool/src/main.rs:657

  • Parent locator precedence is relative, absolute Win32, then volume, and the latter two are Windows-only (as implemented in vm/devices/storage/disklayer_vhdx/src/chain.rs:219-262). Trying volume first can select the wrong existing file and report a linkage mismatch without trying a valid absolute locator; interpreting these strings on Unix can also turn them into unintended local paths.
    if let Some(volume) = parent.volume_path() {
        candidates.push(native_locator_path(volume));
    }
    if let Some(absolute) = parent.absolute_win32_path() {
        candidates.push(native_locator_path(absolute));

vm/devices/storage/vhdxtool/src/main.rs:628

  • A dirty image is treated as a warning only when it is the initially checked file. If a parent is dirty, this open goes through classify_open_error, becomes InconsistentImage, and exits 2, contradicting the documented status-0 dirty-log behavior. Handle LogReplayRequired here consistently (or restructure the loop so every chain element uses the same open path).
        let parent = VhdxFile::open(parent_file)
            .read_only()
            .await
            .map_err(|error| classify_open_error(&parent_path, error))?;

vm/devices/storage/vhdxtool/src/main.rs:629

  • Chain validation checks the linkage GUID but not the child/parent virtual-size invariant that create_image enforces. A same-linkage parent with a different disk size is therefore reported OK even though it cannot back the child's full address space. Compare parent.disk_size() with image.disk_size() before accepting this chain element.
        if parent.data_write_guid() != linkage {

vm/devices/storage/vhdxtool/README.md:4

  • This adds a user-facing developer utility, but only a crate-local README documents it. Repository documentation policy requires a developer-tool page under Guide/src/dev_guide/dev_tools/, an entry in Guide/src/SUMMARY.md, and the corresponding code-sync mapping so the tool remains discoverable and maintained.
`vhdxtool` is a cross-platform command-line utility for creating, inspecting,
validating, replaying, mapping, and converting VHDX images.

vm/devices/storage/vhdx/src/create.rs:199

  • Fixed currently only sets leave_blocks_allocated; creation still writes an all-zero BAT and sizes the file only through the BAT region. The resulting image is dynamically allocated despite being reported as fixed, so create --type fixed and fixed conversion do not satisfy their CLI contract. Fixed creation must allocate every payload block, mark each payload BAT entry fully present, and extend/provision the backing file; add a test that verifies the BAT and file layout rather than only the flag.
    let leave_blocks_allocated = matches!(params.disk_type, DiskType::Fixed);
  • Files reviewed: 16/17 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread vm/devices/storage/openvmm-img/src/main.rs
Comment thread vm/devices/storage/openvmm-img/src/main.rs
Copilot AI review requested due to automatic review settings September 15, 2026 17:02
@github-actions github-actions Bot added the Guide label Sep 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Conversion can destroy its input, and differencing-chain creation and validation currently accept incompatible sector geometry.

Review details

Suppressed comments (6)

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

vm/devices/storage/vhdxtool/src/main.rs:340

  • Differencing creation only checks the virtual size. If the parent uses 4096-byte logical sectors and --logical-sector-size is omitted, the child is defaulted to 512; an explicitly different value is also accepted. Such a chain is unusable because LayeredDisk rejects mismatched logical sector sizes (vm/devices/storage/disk_layered/src/lib.rs:233-238). Inherit the parent's value when omitted and reject a conflicting override.
    vm/devices/storage/vhdxtool/src/main.rs:640
  • check verifies linkage but accepts a parent whose logical sector size differs from the child. The resulting chain cannot be attached because LayeredDisk rejects mismatched sector sizes (vm/devices/storage/disk_layered/src/lib.rs:233-238), so this should be reported as inconsistent here.
    vm/devices/storage/vhdxtool/src/main.rs:474
  • The human-readable output omits volume_path, even though it is a supported parent locator and is preferred over the absolute Win32 path during Windows lookup. As a result, info hides the locator that may actually resolve the parent; include it alongside the relative and absolute paths.

vm/devices/storage/vhdxtool/src/main.rs:639

  • A dirty parent takes this generic classification path and exits as an inconsistent image (status 2), while a dirty leaf is reported as a warning with status 0. This contradicts the command's documented dirty-log behavior and makes the result depend on where the dirty image occurs in the chain. Handle LogReplayRequired here the same way as at lines 604-614.
        let parent = VhdxFile::open(parent_file)
            .read_only()
            .await
            .map_err(|error| classify_open_error(&parent_path, error))?;

vm/devices/storage/vhdxtool/README.md:4

  • This adds a standalone developer utility, but its usage is only documented in a crate README, which is not published in the OpenVMM Guide. Add a Guide/src/dev_guide/dev_tools/vhdxtool.md page, link it from Guide/src/SUMMARY.md alongside the other utilities, and add the code-to-Guide mapping so future CLI changes stay synchronized.
`vhdxtool` is a cross-platform command-line utility for creating, inspecting,
validating, replaying, mapping, and converting VHDX images.

vm/devices/storage/vhdxtool/src/main.rs:727

  • The conversion does not reject input and output paths that identify the same file. With --force, each output path opens with truncation after the input is opened, so converting in place (including through a symlink or hard link) destroys the source before it is copied. Detect file identity and fail before any output creation.
    anyhow::ensure!(
        !matches!(disk_type, DiskType::Differencing),
        "convert output cannot be differencing without a parent"
    );
  • Files reviewed: 18/19 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 15, 2026 18:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

In-place operations can destroy source images, and several parent-chain invariants are not enforced.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

vm/devices/storage/openvmm-img/src/main.rs:350

  • Reject creating the child over its parent before calling BlockingFile::create. With --force, openvmm-img create parent.vhdx --type differencing --parent parent.vhdx opens and reads the parent successfully, then truncates that same file and replaces it with a child pointing to the now-destroyed parent. The check should account for aliases such as symlinks/hard links, not only textual path equality.
    if let Some(parent_path) = options.parent {
        let parent_file = BlockingFile::open(&parent_path, true)

vm/devices/storage/openvmm-img/src/main.rs:757

  • Reject conversions where input and output identify the same file before opening or creating the output. In every conversion branch, --force eventually opens the output with truncate(true) while the input descriptor is still in use, so converting a path onto itself (or through an alias) destroys the source and then fails or reads the newly truncated contents.
    anyhow::ensure!(

vm/devices/storage/openvmm-img/src/main.rs:662

  • check validates only linkage, so it can report OK for an unusable parent/child geometry. A differencing child cannot be larger than its parent, and parent and child logical sector sizes must match; LayeredDisk also rejects sector-size mismatches (vm/devices/storage/disk_layered/src/lib.rs:233-238). Validate these relationships before advancing to the parent.
        if parent.data_write_guid() != linkage {
  • Files reviewed: 18/19 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread vm/devices/storage/vhdx/src/create.rs
Comment thread vm/devices/storage/openvmm-img/src/main.rs Outdated
Comment thread vm/devices/storage/openvmm-img/src/main.rs
Comment thread vm/devices/storage/vhdx/src/error.rs Outdated
Comment thread vm/devices/storage/openvmm-img/README.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Conversion can destroy its input, dirty parents are misclassified, and pathless differencing metadata remains representable.

Review details

Suppressed comments (3)

vm/devices/storage/openvmm-img/src/main.rs:768

  • With --force, nothing prevents the input and output from naming the same file. Every branch opens the input first and then calls a create path that truncates the output, so convert disk.raw -o disk.raw --force destroys the source before copying (aliases such as symlinks can do the same). Reject identical file identities before any output is created, or write to a temporary file and atomically replace the destination after a successful conversion.
    anyhow::ensure!(

vm/devices/storage/openvmm-img/src/main.rs:664

  • A dirty leaf is handled as a warning with success at line 631, but a dirty parent reaches classify_open_error here and becomes InconsistentImage (exit 2). The same condition therefore changes status solely based on chain position. Handle LogReplayRequired here using the same warning path, or restructure the loop so each parent is opened once by the existing handler.
        let parent = VhdxFile::open(parent_file)
            .read_only()
            .await
            .map_err(|error| classify_open_error(&parent_path, error))?;

vm/devices/storage/vhdx/src/create.rs:61

  • MS-VHDX section 2.6.2.6.3 requires at least one relative_path, volume_path, or absolute_win32_path, but this constructor produces a valid VhdxParent with all three absent and create serializes it. This permits nonconforming differencing images that the automatic chain opener cannot resolve. Require at least one locator path before accepting DiskType::Differencing, or model a separate explicitly-resolved parent type that is not serializable as a standalone VHDX.
        Ok(Self {
            linkage,
            relative_path: None,
            volume_path: None,
            absolute_win32_path: None,
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@jstarks
John Starks (jstarks) marked this pull request as ready for review September 15, 2026 23:16
Copilot AI review requested due to automatic review settings September 16, 2026 17:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

In-place operations can truncate source or parent images, and dirty parents are misclassified during chain validation.

Review details

Suppressed comments (4)

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

vm/devices/storage/openvmm-img/src/main.rs:498

  • The human-readable output omits volume_path, even though a valid Windows differencing disk may use that as its only usable parent locator (and it is emitted in JSON and tried by candidate_paths). Include a volume-parent line so info does not hide the actual parent path.

vm/devices/storage/openvmm-img/src/main.rs:410

  • When --force is used and the output names the parent file (including through an alias), BlockingFile::create truncates the already-open parent and replaces it with a differencing disk whose locator points back to itself. Reject outputs that identify the same file as the parent before opening the destination destructively.
    let file = BlockingFile::create(&options.file, options.force)
        .with_context(|| format!("failed to create {}", options.file.display()))?;

vm/devices/storage/openvmm-img/src/main.rs:664

  • A dirty parent is handled differently from a dirty leaf: this eager parent open routes LogReplayRequired through classify_open_error, producing an inconsistency error and exit status 2, while lines 629-637 document and implement dirty images as warnings with success. Handle this error kind here the same way so check has consistent behavior across the whole chain.
        let parent = VhdxFile::open(parent_file)
            .read_only()
            .await
            .map_err(|error| classify_open_error(&parent_path, error))?;

vm/devices/storage/openvmm-img/src/main.rs:780

  • With --force, allowing the input and output to identify the same file truncates the source when the destination is created. Raw input is then read back from the newly written VHDX, and VHDX input loses its payload, so an in-place conversion corrupts data. Reject identical files before either output branch creates the destination.
    match input_format {
  • Files reviewed: 20/21 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions

Copy link
Copy Markdown

Copilot AI review requested due to automatic review settings September 16, 2026 21:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Aliased paths can destroy source or parent images, while dirty parent logs receive inconsistent exit handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

vm/devices/storage/openvmm-img/src/main.rs:768

  • When --force is used, output_path can name the same file as input_path (including through a symlink or hard link). Each conversion branch opens the input first and then truncates the output, so this destroys the source before it is copied. Reject source/output aliases using file identity before any destination create/truncate operation.
    anyhow::ensure!(

vm/devices/storage/openvmm-img/src/main.rs:664

  • A dirty log is treated as a warning with success when the dirty image is current_path, but a dirty parent reaches this call and classify_open_error turns LogReplayRequired into an inconsistent-image error (exit 2). Handle this open with the same warning path so check results do not depend on the image's position in the chain.
        let parent = VhdxFile::open(parent_file)
            .read_only()
            .await
            .map_err(|error| classify_open_error(&parent_path, error))?;

vm/devices/storage/openvmm-img/src/main.rs:410

  • A differencing create with --parent equal to the output and --force opens the parent, then truncates that same file here and replaces it with a child whose locator points back to itself. Reject parent/output aliases before opening the destination so this cannot destroy the parent or create a self-cycle.
    let file = BlockingFile::create(&options.file, options.force)
        .with_context(|| format!("failed to create {}", options.file.display()))?;
  • Files reviewed: 20/21 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment on lines +511 to +512
async fn collect_map(image: &VhdxFile<BlockingFile>) -> Result<Vec<MapRun>> {
let mut runs = Vec::new();
@github-actions

Copy link
Copy Markdown

@smalis-msft

Copy link
Copy Markdown
Contributor

What does this get us that existing tools, like qemu-img, don't? What's the long term plan for this tool? Can this be added to the description?

@jstarks

Copy link
Copy Markdown
Member Author

The immediate gap is VHDX differencing disks, which qemu-img does not support. Longer term we will want a tool that works well with all the formats that OpenVMM supports, which will be a different set from what qemu supports.

qemu-img is still a great tool in general.

Comment on lines 147 to +160
@@ -155,3 +156,5 @@ Each row in the mapping table follows:
update `reference/openvmm/management/cli.md`, and
`dev_guide/contrib/openvmm_packaging.md` when the packager-facing contract
changes.
- Changes to `openvmm-img` commands, options, format support, or behavior must
update `user_guide/openvmm-img.md`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we replace this with something telling agents to just scan the whole guide? There's no way we'll remember to keep this table up to date always.

block_size,
has_parent: true,
disk_type: vhdx::DiskType::Differencing(
vhdx::VhdxParent::new(Guid::new_random()).unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be zeros to make it clearer that a parent hasn't been associated yet?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applies throughout

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this should be moved into a new VhdxParent constructor

}

#[derive(Clone)]
pub(crate) struct BlockingFile {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much of petri/src/disk_image.rs can this tool eventually replace? I think most of it.

#[arg(long = "type", value_enum, default_value = "dynamic")]
disk_type: DiskType,
/// Parent VHDX path. Required for, and only valid with, differencing images.
#[arg(long)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to express this validity requirement entirely within clap syntax?

Comment on lines +124 to +126
/// VHDX payload block size. Must be a multiple of 1 MiB between 1 MiB
/// and 256 MiB. Defaults to 2 MiB. Accepts binary size suffixes.
#[arg(long, value_parser = util::parse_size)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about validity, and default value can be specified with clap

Comment on lines +128 to +133
/// Logical sector size in bytes: 512 or 4096.
#[arg(long)]
logical_sector_size: Option<u32>,
/// Physical sector size in bytes: 512 or 4096.
#[arg(long)]
physical_sector_size: Option<u32>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about validity, or maybe these should be an enum? Will these be the only values supported for other formats in the future?

Comment on lines +134 to +136
/// Alignment of the VHDX data region. Accepts binary size suffixes.
#[arg(long, value_parser = util::parse_size)]
block_alignment: Option<u64>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move everything VHDX specific into a VHDX-only type somehow, if possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applies to comments too

Comment on lines +165 to +181
#[arg(short, long)]
output: PathBuf,
/// Source format. Inferred from a .vhdx extension when omitted.
#[arg(long, value_enum)]
input_format: Option<ImageFormat>,
/// Format of the converted image.
#[arg(long, value_enum)]
output_format: ImageFormat,
/// Allocation type for VHDX output.
#[arg(long = "type", value_enum, default_value = "dynamic")]
disk_type: DiskType,
/// Payload block size for VHDX output. Must be a multiple of 1 MiB
/// between 1 MiB and 256 MiB. Defaults to 2 MiB. Accepts binary size
/// suffixes.
#[arg(long, value_parser = util::parse_size)]
block_size: Option<u64>,
/// Replace the output file if it already exists.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some set of flags that Convert and Create could share by moving them into a new flattened struct?

force: bool,
}

async fn create_image(options: CreateOptions) -> Result<()> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move vhdx and/or raw specific logic to be behind a per-format trait, and then live in their own separate files.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks cool. My one significant thought is that I think we really should be spending time getting our CLI interfaces right and using all of clap's features to their fullest, before anybody even thinks about taking a dependency on our interfaces. Plus we can use those learnings to start thinking about refactoring our bigger CLIs, like openvmm's.

@smalis-msft

Copy link
Copy Markdown
Contributor

Lets also make sure #4481 gets merged first, and this PR can be rebased on top of it and add a guide page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Guide unsafe Related to unsafe code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants