Differencing phase 13: guest VHDX sector-bitmap read path¶
Prompt¶
Plan phase 13 of PLAN-differencing.md, the second of the two guest
read-path phases: teach the guest to compose a differencing VHDX child
against its parent at logical-sector granularity, so that a
PAYLOAD_BLOCK_PARTIALLY_PRESENT block reads each sector from
whichever file owns it rather than failing the read.
Phase 12 landed the VHD half (b4ae7fca, #615). The two formats are
separate phases because they are not the same problem: VHD carries a
512-byte bitmap in front of every block, while VHDX keeps bitmaps in
their own 1 MiB blocks reached through interleaved BAT entries, counts
sectors in logical-sector units that may be 4096 bytes, and orders its
bitmap bits the other way round. Nothing on the host builds a composing
VHDX chain yet -- phase 14 is what changes that -- so this is a
crate-level change verified by crate-level tests. The phase plan is the
deliverable; implementation is a separate ask.
Planning effort¶
High. Same reasoning as phase 12: a guest read path in no_std
code where a wrong answer is silently wrong data rather than a crash.
Two things make it no easier than phase 12 despite the precedent. The
bitmap is reached through a different structure, so the "find the
bitmap" half is new work rather than a port. And the three differences
from VHD -- bit order, sector unit, and a bitmap block that may be
legitimately absent -- are each individually invisible to a test that
happens to use the symmetric case, which is exactly the class of
mistake phase 12 spent two review rounds closing.
Review effort: high, for the same reason. The master plan does not specify one for this phase.
Scope¶
In scope.
- A sector-bitmap reader in
src/crates/vhdx/src/lib.rs: resolve a payload block's interleaved sector-bitmap BAT entry, read bitmap bytes through the state's existing data cache, and coalesce runs of same-owner logical sectors. - The
PAYLOAD_BLOCK_PARTIALLY_PRESENTcase of the VHDX arm ofread_chain_virtual_cluster(src/crates/qcow2/src/lib.rs:10269): classify a chunk, serve a wholly-owned chunk with one read, and compose a mixed one. - Separating
NotPresentfromZeroin that arm. They are currently one match arm, which is wrong for a differencing child -- see F3. - Failing closed, on every path that could otherwise serve a parent-owned sector with no parent behind the child, exactly as the VHD arm does.
- Adding
vhdx-inputto the lint and unit-test feature matrix (issue #616), without which none of the above is compiled by either target. - Crate-level tests, including a VHDX mock-chain harness.
Out of scope.
- Lifting
init_chain_states' refusal of a differencing VHDX (src/crates/qcow2/src/lib.rs:11298). It stays unconditional, for the reason phase 12 established and issue #614 records:device_countbounds a flat array that may hold more than one chain, so "a device follows this one" does not mean "this child has a parent". Phase 14 owns that, and needs #614 settled first. - Any operation reaching the new path. No host change at all.
map's VHDX partial-present walk (docs/map.md:238). That is a different consumer of the same structure and a separate change; this phase must not silently make that limitation note false, and the definition of done says so.- Integration tests and fuzzing (phases 15), documentation (phase 16).
PAYLOAD_BLOCK_UNDEFINED/UNMAPPEDsemantics beyond what the current code already does. They are not differencing-specific.vhdx::calculate_bat_layout's writer-side BAT entry count (src/crates/vhdx/src/lib.rs:3301), used bycreate,convert,measureandresize. It sizes a differencing image's BAT by the same shorter rule 13f's reader-side fix works around rather than widens (see F10/F11 below), so every differencing VHDX instar writes still has unreachable sector-bitmap entries past the first partial chunk group. Fixing it changes the on-disk layout of every such image and needs each of its four callers checked on its own. Tracked as issue #623, deliberately not fixed here.- Refusing a block offset that overlaps the BAT or metadata region.
Review round 3 raised it as a
consider, and it is the same "invent an answer for a malformed image" shape as the offset-zero case F15 closed -- but that one is a single comparison against a constant, where this needs the declared region bounds retained onVhdxStateand applied at three sites, and raises the question of whetherscan_allocationandmap_extentsshould do the same. Tracked as issue #625. - A compose test at 4096-byte logical sectors beyond the first chunk
group. Also review round 3, also a
consider. At that geometrychunk_ratiois 32768 rather than 4096, so the padded bound and the group stride are arithmetic the current tests only drive at the 512 geometry; the fixture BAT region is large enough that the test would be cheap. It is coverage of an existing property rather than a defect, and phase 15 is the tests-and-fuzz phase, so it belongs there -- recorded here so phase 15 does not have to rediscover it.
What the survey found¶
Surveyed on 2026-10-04 against develop at 758ffba8, with phase 12
merged. Verified by running commands rather than by reading, where a
command existed.
F1. The VHDX arm is compiled by neither make lint nor
make test-rust -- measured, not inferred. Issue #616 says so;
this is the proof. With a deliberate type error injected into the VHDX
arm, make lint exits 0. Adding vhdx-input to the two feature lists
(Makefile:536, scripts/check-rust.sh:134,140) and re-running gives
error[E0308] naming the injected line. This is the same gap phase 12
found for VHD (its F12) and deliberately did not close for VHDX,
because this phase was going to rewrite that code.
F2. Unlike the VHD arm, the VHDX arm already compiles clean. With
vhdx-input added to both lists and no probe, make lint exits 0 with
no clippy findings, and make test-rust reports 2383 passed / 0 failed
-- identical to the baseline on develop. Phase 12's equivalent step
surfaced real clippy findings on first compile; this one will not. The
feature addition also buys zero new tests, because there is no
VHDX test in the qcow2 crate at all: vhdx appears ten times in that
11,000-line file and every occurrence is production code. So step 13a
is a one-line matrix change that makes later steps' code visible, not a
cleanup job, and it provides no coverage by itself.
F3. The VHDX arm conflates "not present" with "explicitly zero", and
that is wrong for a differencing child. At
src/crates/qcow2/src/lib.rs:10275 the arm reads:
and VhdxBlockLookup::NotPresent's own doc comment says "(reads as
zero)". For a file with no parent the two are indeed the same answer,
which is why this has never been wrong. For a differencing child they
are opposites: PAYLOAD_BLOCK_NOT_PRESENT means the data lives in the
parent, and PAYLOAD_BLOCK_ZERO means this block is zeros and the
parent must not be consulted. Composing without separating them would
read the parent's data where the child says zero. The master plan does
not mention this; it is this survey's main find, and it is a
correctness item rather than a structural one.
F4. PARTIALLY_PRESENT is already a deliberate, documented refusal,
and it is the phase's subject. block_lookup
(src/crates/vhdx/src/lib.rs:1984) returns None for state 7, with a
comment saying it is a backstop for a caller that skipped the
has_parent check. The arm turns None into return false. So the
starting position is fail-closed rather than silently-wrong, which is a
better starting point than the VHD arm had.
F5. VhdxState already carries everything the reader needs except
the bitmap itself. logical_sector_size (512 or 4096, validated at
src/crates/vhdx/src/lib.rs:1818), block_size, chunk_ratio
(computed at :1823 as (2^23 * logical_sector_size) / block_size),
total_bat_entries, bat_offset, and has_parent are all fields of
the struct at :1641. block_lookup already computes the interleave
correction, sb_entries_before = block_index / chunk_ratio, at
:2001. What is missing is reading the SB entry rather than skipping
past it.
F6. The vhdx crate has an allocated, never-used data cache -- the
same free resource phase 12 found for VHD. data_cached_sector and
data_cache_buf are fields of VhdxState (:1669-1670), are assigned
at init (:1864-1865), and are read nowhere: grep -c data_cache_buf
src/crates/vhdx/src/lib.rs returns 4, all declaration or assignment.
The VHD crate's count went from 8 (all unused) to 11 (used) over phase
12. Bitmap reads can use this cache, adding no guest memory, exactly as
phase 12's did.
F7. The format differences from VHD are already pinned against real
oracles, and so is a fixture generator. Phase 1 measured them
(docs/plans/PLAN-differencing-phase-01-pin.md):
- The VHDX sector bitmap is least significant bit first, "the
opposite of VHD" (
:1463). VHD isbit (7 - i % 8); VHDX isbit (i % 8). - A bit counts one logical sector, which may be 4096 bytes, not a fixed 512 as in VHD.
- A sector-bitmap block is 1 MiB, hence
chunk_ratiopayload blocks per bitmap block. - SB BAT entry states are
SB_BLOCK_NOT_PRESENT(0) andSB_BLOCK_PRESENT(6) (:430-431). - An SB entry "may only be
SB_BLOCK_NOT_PRESENTif no associated payload block isPAYLOAD_BLOCK_PARTIALLY_PRESENT" (:878-885), so aPARTIALLY_PRESENTblock whose bitmap block is absent is a malformed image. vhdx_sector_bitmap_block()andpatch_vhdx_child()at:1810and:1819build such an image in Python;instar-testdatacarries the resultingvhdx-diff-child.vhdx/vhdx-diff-parent.vhdxpair.
Set bit means the sector lives in this file, same polarity as VHD.
F8. Neither SB_BLOCK_NOT_PRESENT nor SB_BLOCK_PRESENT exists as a
constant in the vhdx crate. grep -n 'SB_BLOCK' src/crates/vhdx/src/lib.rs
returns nothing, while the six PAYLOAD_BLOCK_* constants are defined
at :197-207. The crate's BAT walkers skip SB entries by position
without ever looking at their state.
F9. Phase 12's final shape differs from its mid-phase shape, and
phase 13 should copy the final one. Review round 3 (4a7a143f)
removed the read_cluster_sectors / read_offset_sectors branch from
read_vhd_child_runs: every child run now goes through
read_offset_sectors with the caller's scratch, because the aligned
branch still took the 64 KiB-stack path whenever a run was not a whole
number of device sectors. A VHDX reader written by copying the
mid-phase VHD code would reintroduce that. The same round renamed the
regression guard to vhd_arm_dynamic_reads_as_a_plain_dynamic_vhd.
Corrections made at source. The master plan's phases 12-and-13
bullet cites pre-phase-12 line numbers throughout
(read_chain_virtual_cluster at :7930, the format dispatch at
:8486, ChainStates at :9385, the subcluster path at
:8106-8130, the refusals at :9528 and :9556). Phase 12 added
about 2,800 lines to that file and every one of them is now wrong. The
planning commit corrects the two a phase 13 reader will follow -- the
VHDX refusal, now :11298, and the VHDX arm, now :10269 -- and
marks the rest as pre-phase-12. No later step needs to redo this.
Nothing else the master plan says about phase 13 was found to be false.
The findings below were made executing the plan above, not planning it, and are added here rather than left to be inferred from commit messages.
F10. The risk list's claim that the NotPresent/Zero split
"should not" change non-differencing reads (below, "Risks and
mitigations") is wrong, in exactly one case that is unreachable
rather than harmful. 13c makes Zero zero-fill and return
immediately, where before the split it fell through to continue
with NotPresent and reached the walk's zero-fill tail by the same
route. cc0989a0's regression guard compares five reads of a
parentless image against 758ffba8 and finds four identical, with
the third -- an explicitly zeroed block with a device behind it --
changed: it used to descend into that device and now reads as
zeros directly. No chain a host builds can produce this shape, since
a parentless image has no backing file to put behind it, so the
difference cannot reach a real read today. The guard drives the case
anyway, to pin the new answer rather than leave it untested, and that
is what shows the risk list's claim to be false rather than merely
unproven.
F11. A differencing VHDX's sector-bitmap BAT entries are
unreachable whenever its block count is not a whole number of chunk
groups, and the master plan does not anticipate this at all.
VhdxState::init sized total_bat_entries by the rule for an image
with no parent -- one entry per payload block plus one per group --
and sector_bitmap_lookup bounded itself by that same count. A
differencing image's BAT is actually padded to whole groups of
chunk_ratio + 1 entries, because a group's sector-bitmap entry sits
at the end of the group whether or not every payload entry ahead of
it is backed by virtual disk. Any differencing image whose block
count is not a whole number of groups therefore had at least one
unreachable bitmap entry, and one smaller than a single group -- 4
GiB at the common 1 MiB block / 512-byte sector geometry -- had none
reachable at all. The project's own 16 MiB vhdx-diff-child.vhdx
fixture is the second case, so this blocked the phase's own
deliverable: the composing reader built in 13c could not have read
instar's real fixture. Found by 13d's implementer noticing that
every fixture it had managed to get working used a whole number of
chunk groups, not by anything in this plan. Fixed in ec005d57 by
adding a separate bound, sb_bat_entry_bound, computed by the padded
rule for a differencing image and capped by what the declared BAT
region holds. total_bat_entries itself is deliberately left at the
shorter count, because it also sizes the whole-BAT walks in
scan_allocation and map_extents; widening it would make those
walk past a BAT region a writer still sizes by the shorter rule --
the writer-side half of this same defect, left in place as issue
623 (see Scope, above).¶
F12. A parentless image could reach the composing arm, and the
answer it got there was invented. Found by review round 1 on the
phase's pull request, not by this plan or by any step of it.
block_lookup returned PartiallyPresent for BAT state 7 whatever
has_parent said, and the arm composed it without rechecking. Nothing
upstream stops such an image: the differencing refusal in
init_chain_states fires on has_parent, which is exactly what the
image denies having, and sb_bat_entry_bound falls back to
total_bat_entries for it, which is wide enough to resolve the first
group's bitmap entry. So a crafted image with HasParent clear, a
state-7 payload entry and a present bitmap got a composed read where
before this phase existed it got a failure -- state 7 had no arm at
all and fell through the lookup's catch-all. That is the shape issue
547 is about: inventing an answer for a malformed image. The doc¶
comment written in 13c asserted the opposite ("One that has not
cannot resolve the bitmap either, so it still fails rather than
inventing data"), which made it the wrong kind of wrong -- a claim a
reader would rely on. Fixed by refusing state 7 in block_lookup
when has_parent is clear, which is where the refusal belongs
because the crate is the authority on what each state means for each
kind of image.
F13. The phase's mutation evidence was not reproducible from the
tree, and neither was phase 12's. Also from review round 1. Phase
12's and phase 13's commit messages both describe mutations "kept in
a runnable script", and tools/mutate-differencing.sh is that script
by name -- but its 26 cases all mutate the writer, and the reader
mutations for both phases lived only in a session scratchpad. The
claim was therefore unfalsifiable by anyone reading the repository.
Corrected by committing reader cases to that harness in 13h and
13i below, covering phase 12's VHD set as well as phase 13's VHDX
one: the gap was the same gap, and fixing only this phase's half
would have left the next phase's review to find the other.
F14. Two properties were pinned by the wrong test, and one guard
was not a guard. Found by the first full run of the harness once
the reader cases were in it, which is the point of putting them
there. Three cases named an arm test that did not kill their
mutation. Two were a mapping error: a coalescer that never advances
its bitmap byte, and a block-end guard admitting one sector too
many, are both invisible at the arm, because the arm re-enters the
coalescer once per ownership run with a correct starting byte and
refuses an over-long chunk by a second route. Those properties are
the crate's, so the cases now name the vhd and vhdx tests that do
pin them. The third was not a mapping error: sector_bitmap_lookup
checked state == SB_BLOCK_NOT_PRESENT and then
state != SB_BLOCK_PRESENT, and nothing can reach the second
through the first, so removing the earlier branch changed no
behaviour and no test could kill it. It read as two guards where
there was one. Collapsed into a single comparison whose comment
carries both reasons.
F15. A present BAT entry naming file offset zero was believed, three times over. From review round 2. A BAT entry is a state in its low bits and a megabyte-granular offset in the rest, so a zeroed or truncated entry whose state bits happen to read as present names offset 0 -- which is the file identifier, not a block. Nothing checked it, for fully present payload blocks, partially present ones or sector bitmaps alike, so each would have served header bytes as though they answered the question asked: what is in this block, or which of its sectors does the child own. The review raised the sector-bitmap case; the other two are the same invariant and were found by looking for it. Fixed with one named constant and a comparison at each of the three sites, which is cheap because the offset mask already clears the low twenty bits and so zero is the only value below the floor.
Two smaller things came with it. sectors_per_chunk_group returned
Some(0) for a chunk ratio of zero, contradicting its own doc
comment, and a zero is a number every bounds check downstream
accepts -- the shape of a "cannot tell" answer dressed as a real
one. It returns None now. And the VHDX arm had no three-device
test, so the vhdx_states[dev_idx] re-borrow after the parent
recursion could have been pinned to zero undetected; the VHD side
pins the same property with a test and a mutation, and the VHDX side
now does too.
Decisions¶
-
Mirror phase 12's three-part shape rather than inventing one. A lookup that resolves where the bitmap lives, a run coalescer over the bitmap, and a classify-then-serve arm. Phase 12 arrived at this shape under two review rounds and it is now the house pattern for this exact problem; a second, different shape in the same function would be worse than a slightly imperfect fit. Concretely:
differencing_block_lookup→sector_bitmap_lookup,read_sector_bitmap_run→read_vhdx_sector_bitmap_run,classify_vhd_chunk_ownership→classify_vhdx_chunk_ownership,read_vhd_child_runs→read_vhdx_child_runs. -
Do not generalise phase 12's VHD code into shared helpers. The differences are not parameters: a different structure locates the bitmap, the bit order is reversed, the sector unit is variable, and the bitmap block can be legitimately absent. A shared helper would carry four conditionals and would make each format's reader harder to check against its own spec. The coalescer is the one piece that looks genuinely common --
coalesce_ownership_run(src/crates/vhd/src/lib.rs:1401) already takes a closureFnMut(u32) -> Option<u8>returning a bitmap byte -- and even there the bit order differs, so the VHDX reader passes its own bit-extraction. Revisit sharing in phase 15, with both readers written and both test suites available to prove the refactor invisible, not now. -
Separate
NotPresentfromZeroin the arm, for every VHDX, not only differencing ones (F3).Zerozero-fills the chunk and stops;NotPresentdescends to the next device. For a non-differencing VHDX this is behaviour-identical, because a chain never has a device behind a parentless image and both answers reach the zero-fill tail. The alternative -- gate the split onhas_parent-- keeps a second code path alive to no purpose and leaves the wrong doc comment in place. The regression test must prove the identical-behaviour claim rather than assert it. -
Fail closed at the bottom of a chain on all three parent-owned paths, as phase 12 does:
NotPresentwith no device behind a child whosehas_parentis set, a wholly parent-ownedPARTIALLY_PRESENTchunk, and the parent-owned runs of a mixed one. Usedevices_behind()(src/crates/qcow2/src/lib.rs:9419), which phase 12 added for exactly this and which already fails closed on an impossible offset. -
A
PARTIALLY_PRESENTblock whose SB entry isSB_BLOCK_NOT_PRESENTfails the read. F7 records that the spec forbids the combination. The alternative readings -- treat the absent bitmap as all-child or all-parent -- each invent an answer for a malformed image, and inventing answers for malformed images is what issue #547 was. The same applies to an SB entry in any state other than 0 or 6. -
Refuse a chunk that reaches past the end of the block its bitmap describes, as the VHD arm does. This is the asymmetry phase 12's F13 recorded: the older non-differencing path does not cap at the block boundary and issue #613 tracks it. Phase 13 adds no new instance of that defect and does not fix the old one.
-
Test at crate level with a mock chain; no testdata fixture. Same as phase 12's decision 7 and for the same reason: no host path builds a composing VHDX chain until phase 14, so an integration test cannot reach this code. Phase 15 owns the fixture-based cross-validation. The mock harness should be a VHDX sibling of
run_vhd_chain_read, not a generalisation of it -- see decision 2. -
The bitmap fixture builder takes an explicit logical sector size and an explicit bit list. Phase 12's review found that single-shape fixtures hide whole classes of mistake: every early fixture had a one-byte bitmap, so no arm test crossed a bitmap byte, and every fixture had one block, so no test resolved a BAT entry for block 1. Build the general fixture first this time.
Step plan¶
| Step | Effort | Model | Isolation | Brief for sub-agent |
|---|---|---|---|---|
| 13a | low | sonnet | none | Add vhdx-input to the two feature lists that gate the qcow2 crate's optional input readers: the cargo test --release -p qcow2 line in Makefile's test-rust target (:536), and both cargo clippy -p qcow2 invocations in scripts/check-rust.sh (:134 and :140, the fix and check branches). The lists currently end dmg-input,vhd-input. Demonstrate the change reaches the code by injecting a deliberate type error into the VHDX arm of read_chain_virtual_cluster (src/crates/qcow2/src/lib.rs:10269), confirming make lint fails with error[E0308] naming that line, and removing the probe; state in the commit message that you did this and what it printed. F2 established that no clippy findings and no new tests follow, so do not expect either -- if clippy does report something, that is new since 2026-10-04 and worth saying so. Closes issue #616; use the Fixes #616 keyword. |
| 13b | high | opus | none | Add the sector-bitmap reader to src/crates/vhdx/src/lib.rs. Three pieces. (1) pub const SB_BLOCK_NOT_PRESENT: u64 = 0; and pub const SB_BLOCK_PRESENT: u64 = 6; beside the PAYLOAD_BLOCK_* constants at :197-207 (F8: neither exists today). (2) A sector_bitmap_lookup method on VhdxState (:1641) that, for a virtual offset, returns the host byte offset of the block's sector-bitmap block and the index of the first logical sector of the chunk within it, or None. The BAT index of the SB entry for payload block b is (chunk_ratio + 1) * (b / chunk_ratio) + chunk_ratio; block_lookup at :2001 already computes the payload side of the same interleave and is the model to follow. Validate the SB entry's state: SB_BLOCK_PRESENT proceeds, anything else returns None (decision 5). Bit n of the bitmap block covers logical sector n of the chunk group, not of the block, so the sector index is relative to the group's first block. (3) A run coalescer. Read bitmap bytes through data_cached_sector / data_cache_buf, which are allocated and currently unused (F6) -- follow read_u64_le_cached's cached-sector pattern. The bit order is the opposite of VHD: sector i is bit i % 8 of byte i / 8, least significant first, measured in phase 1 (F7). A set bit means the sector lives in this file. A sector is logical_sector_size bytes, which is 512 or 4096 -- not a constant. coalesce_ownership_run in the vhd crate (src/crates/vhd/src/lib.rs:1401) is the shape to copy, not to call (decision 2). Unit-test the coalescer as a pure function with both sector sizes, a bitmap spanning several bytes, and runs that start and end mid-byte. |
| 13c | high | opus | none | Teach the VHDX arm of read_chain_virtual_cluster (src/crates/qcow2/src/lib.rs:10269) to compose. Two separable changes; make them two commits if it reads better. First, split the NotPresent | Zero arm (F3, decision 3): Zero zero-fills chunk_size bytes and returns true, NotPresent continues to the next device, and for a child whose has_parent is set with no device behind it, NotPresent fails closed via devices_behind() (:9419, decision 4). Fix VhdxBlockLookup::NotPresent's doc comment, which says "(reads as zero)". Second, add the PARTIALLY_PRESENT case: block_lookup (src/crates/vhdx/src/lib.rs:1984) returns None for state 7 today, so it needs a new VhdxBlockLookup variant carrying the block's file offset; classify the chunk with 13b's reader, serve an all-child chunk with the existing single read, fill an all-parent chunk by recursing into the chain, and for a mixed chunk fill from the parent and then overwrite the child's runs. Refuse a chunk reaching past the block boundary (decision 6). Read every child run through read_offset_sectors with the caller's scratch, never read_cluster_sectors -- phase 12's third review round removed exactly that branch because the aligned path still put a 64 KiB buffer on the guest stack whenever a run was not a whole number of device sectors (F9); read_vhd_child_runs on develop is the correct model, ed4f2669's version is not. |
| 13d | high | opus | none | Crate-level tests in the qcow2 crate's test module. There is no VHDX harness there at all (F2), so build one: a VHDX sibling of run_vhd_chain_read and a fixture builder that takes the logical sector size, the per-block payload states, and an explicit list of child-owned sector indices (decision 8 -- build the general builder first; phase 12 paid two review rounds for not doing so). The mock device must refuse a read at any sector size but its own, as the VHD mock does, or a whole class of sector-size mistake is invisible. Cover, at minimum: an all-child and an all-parent PARTIALLY_PRESENT block; a mixed one; the same mixed case at logical_sector_size 4096; a run crossing a bitmap byte boundary; a block that is not the first in its chunk group, so the SB interleave arithmetic is exercised; Zero versus NotPresent giving different answers for a child with a parent behind it; each of the three fail-closed paths at the bottom of a chain; a PARTIALLY_PRESENT block with SB_BLOCK_NOT_PRESENT; and a chunk crossing a block boundary. Add a regression test proving decision 3's identical-behaviour claim for a non-differencing VHDX, comparing against develop at 758ffba8. Prove each test by mutation rather than by reading it, keep the mutations in a runnable script, and state the count in the commit message -- phase 12 ended at fifteen and two of its survivors were real findings. The bit-order mutation (i % 8 to 7 - i % 8) and the sector-unit mutation (logical_sector_size to a literal 512) are the two that matter most. |
| 13e | medium | sonnet | none | Bookkeeping. CHANGELOG.md: a sibling of the phase 12 entry at :12 saying the guest chain walker composes a differencing VHDX, and that no operation reaches it yet because init_chain_states still refuses every differencing VHDX. Record what the survey found at its source in docs/plans/PLAN-differencing.md if 13b-13d falsify anything this plan claims. Confirm docs/map.md:238's VHDX partial-present limitation note is still true -- map is a different consumer and this phase does not change it -- and leave it alone if so. Do not touch docs/ otherwise: phase 16 owns the documentation, and phase 14 owns the user-visible change. |
| 13f | high | opus | none | Unplanned, added during execution after 13d found that a differencing image's sector-bitmap BAT entries were unreachable for any block count that is not an exact multiple of chunk_ratio, including the project's own 16 MiB vhdx-diff-child.vhdx fixture (F11, above). VhdxState::init sized total_bat_entries the way an image with no parent is sized -- one entry per payload block plus one per group -- while a differencing image's BAT is padded to whole groups of chunk_ratio + 1 entries, so the last group's sector-bitmap entry, and every entry in a disk smaller than one group, sat past the bound sector_bitmap_lookup checked itself against. Add a separate bound, sb_bat_entry_bound, computed by the padded rule when has_parent is set and capped by what the declared BAT region holds; leave total_bat_entries at the shorter count, because scan_allocation and map_extents size their whole-BAT walks by it and widening it would walk those past a region a writer still sizes by the shorter rule. Do not touch the writer side (calculate_bat_layout, src/crates/vhdx/src/lib.rs:3301) -- that is issue #623, out of scope here (see Scope, above). Add crate-level tests of the entry-count arithmetic and a compose test at the project's own 16 MiB geometry; confirm the compose test fails without the fix and add a mutation reverting the count. Built as ec005d57. |
| 13g | high | opus | none | Unplanned, added during execution. The Definition of done requires a test proving the VHDX differencing refusal fires for its own reason rather than for any init failure, and the 13d brief omitted it -- my error in writing the brief, not the implementer's. Add a send_error recorder to the VHDX harness and assert the operation and status the refusal raises, with dynamic-image controls and a two-device case so the #614 hazard is pinned. Built as 213725a0. |
| 13h | high | opus | none | Unplanned, added in response to review round 1 on the phase's pull request. Three things, and the third is the systemic one. Refuse PAYLOAD_BLOCK_PARTIALLY_PRESENT in block_lookup when has_parent is clear, so a crafted parentless image cannot reach the composing arm and get an invented answer (F12, above). Separate the sector-bitmap state check from the absent-offset case with a test that rewrites a present entry's state in place, keeping a usable bitmap at the offset the entry carries, so the test cannot pass by rejecting offset zero alone; and cover the two sub-sector shapes the arm is written for but no test drove -- a read beginning part way through a logical sector, and a 512-byte-logical-sector image read through a 4096-byte device sector. Then commit the reader mutations to tools/mutate-differencing.sh (F13, above): 31 cases covering phase 12's VHD arm as well as this phase's VHDX one, each naming the one qcow2 test that must kill it and running with the full input-format feature list, plus a rust_survivor_case type for the one mutation documented as not caught -- it asserts survival and fails if the mutation is ever killed, because that would mean the recorded reason has gone stale. Update EXPECTED_CASES and docs/testing.md together, which the harness checks for itself. Then run the harness and act on what it says: the first full run returned four cases the named test did not kill, which became F14 above -- three re-pointed at the crate test that does pin the property, and one guard collapsed because it turned out not to be one. Final state 57 cases, 56 killed and one surviving as documented, in 4m43s on a warm tree. Built as 13h. |
| 13i | high | opus | none | Unplanned, added in response to review round 2. Three of its fix items were stale counts in prose that the round-1 commit had moved -- the harness case total, the reader-case count and the survivor count -- so the fix is not only to correct them but to derive them: check_case_count now counts the reader and survivor cases in the script itself and asserts both against docs/testing.md, the way it already did for the total, and the hand-maintained numbers are gone from the CI workflow comment entirely. Both new checks were confirmed to fire by making each count wrong in turn. Then the four optional items, all taken: refuse a BAT entry naming file offset zero at all three sites that carry an offset, not only the sector bitmap the review named (F15, above); make sectors_per_chunk_group return None for a zero-sector group as its doc comment already claimed; add the three-device VHDX chain test and the dev_idx mutation the VHD side has had since phase 12; and rewrap a CHANGELOG line. Five mutations accompany the new tests, taking the harness to 62 cases. |
Risks and mitigations¶
- The bit order is written the VHD way. This is the single most
likely defect, it is invisible to any symmetric fixture (
0x00,0xFF, or a palindromic byte), and it produces plausible data rather than an error. Mitigation: 13d's fixtures use asymmetric bytes, and the mutation set includes the bit-order flip specifically. The implementer checks the measured statement atdocs/plans/PLAN-differencing-phase-01-pin.md:1463rather than reasoning from the VHD code beside them. logical_sector_size4096 is treated as 512. Every bitmap arithmetic error of this kind still works at 512. Mitigation: 13d requires the mixed case at 4096, and a mutation replacinglogical_sector_sizewith a literal 512 must fail a test. Phase 12 hit precisely this: its first sector-size mutation survived because no test used any sector size but 512.- The SB interleave is computed for block 0 and never for any
other.
(chunk_ratio + 1) * (b / chunk_ratio) + chunk_ratiodegenerates tochunk_ratiowhenb < chunk_ratio, so a fixture with few blocks exercises nothing. Mitigation: 13d requires a block outside the first chunk group. Phase 12's equivalent gap -- every fixture a single block -- was found by review, not by the phase. - The
NotPresent/Zerosplit changes non-differencing reads. It should not, and decision 3 rests on that. Mitigation: 13d's regression test compares againstdevelopat758ffba8; the implementer states the two captured outputs in the commit message. - The arm is written against a reader that is never compiled. 13a is first for this reason, and F1 proves it is load-bearing rather than tidy-mindedness: without it, 13b-13d can be committed broken and CI stays green.
Definition of done¶
make lintandmake test-rustcompile the VHDX arm. Falsifiable: injecting a type error atsrc/crates/qcow2/src/lib.rs:10269makesmake lintfail, where on758ffba8it exits 0. Issue #616 is closed by theFixeskeyword in 13a's pull request body, not only in a commit message.grep -c 'data_cache_buf' src/crates/vhdx/src/lib.rsreturns more than the 4 it returns today -- that is, F6's allocated-but-unused cache is used.grep -c 'SB_BLOCK_PRESENT' src/crates/vhdx/src/lib.rsis non-zero, and no literal6stands in for it at a use site.- A differencing VHDX child over a parent device, with a
PARTIALLY_PRESENTblock whose sector bitmap is mixed, reads each logical sector from the correct device, at both 512 and 4096. A mutation reversing the bit order fails a test, and a mutation replacinglogical_sector_sizewith 512 fails a different one. State in the commit message that both were run and what they printed. - A
PARTIALLY_PRESENTblock whose SB entry isSB_BLOCK_NOT_PRESENTfails the read; so does an SB entry in any state but 0 or 6. PAYLOAD_BLOCK_ZEROandPAYLOAD_BLOCK_NOT_PRESENTproduce different results for a differencing child with a device behind it, and identical results for a VHDX with nothing behind it. Both are asserted by tests, not argued in a comment.- A non-differencing VHDX over a backing device produces byte-identical
output to
developat758ffba8; the two captured outputs are in 13d's commit message. init_chain_statesstill refuses every differencing VHDX unconditionally, and a test asserts the refusal fired for that reason -- by the status reachingsend_error, not by the return value alone. Phase 12'svhd_init_refuses_a_differencing_child_and_admits_a_dynamic_oneis the model, including its positive control.make test-rustpasses with zero failures and the count is stated, against the 2383 this survey measured on758ffba8.- No test in this phase depends on a testdata fixture.
- No source file or comment added by this phase cites a plan phase,
step or decision number. Falsifiable:
git diff 758ffba8..HEAD -- 'src/*' | grep '^+' | grep -iE 'PLAN-[a-z0-9-]+\.md|decision [0-9]|phase 1[0-9]|13[a-e]'is empty. docs/map.md:238's VHDX partial-present limitation is either still true or updated; the pull request says which.CHANGELOG.mdsays the VHDX composition path exists and that no operation reaches it yet.
Back brief¶
Before implementing, confirm back to me:
- The three VHDX-versus-VHD differences, in your own words, and where in 13b each one is handled: bit order, sector unit, and the bitmap block's location and possible absence. Phase 12's review rounds were largely about input shapes nobody had thought of; the cheapest place to catch the equivalent here is before any code is written.
- Whether decision 2 still looks right once you have read both crates' bitmap code. If the coalescer really is shareable with one closure and no conditionals, say so before writing a second copy -- that is the one decision here a reviewer is most likely to argue with, and it is much cheaper to change now than after 13d's tests are written against two readers.
- The SB BAT index formula, checked against a worked example with
chunk_ratio4096 and a block index above 4096. Get this wrong and every test using a small fixture still passes.
Gate: do not start 13d until 13b and 13c are both committed. Phase 12 wrote its harness against mid-phase code and the third review round changed the production shape underneath it; the tests are cheaper to write once the shape has settled.