Re: [Qemu-devel] [PATCH v4 00/14] qemu-img map: Allow driver to return file of the allocated block

[Fam Zheng <famz@redhat.com>]

On Mon, 01/04 22:02, Max Reitz wrote:
> On 24.12.2015 06:50, Fam Zheng wrote:
> > v4: Rebase and resend, adding Eric's and Stefan's reviewed-by.
> > 
> >     Fix one typo in patch 13.
> > 
> >     Drop previous patch 14 for a later rework because it is not a hard
> >     requirement, but it is pending on Eric's QAPI-to-JSON visitor series:
> > 
> >     https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03929.html
> > 
> > v3: Add Eric's rev-by in patches 6, 7, 13, 14.
> >     12: New, split out from the previous 13.
> >     12->13: Refactor "entry_mergable" from imp_map().
> >         Don't mess the merge conditions. [Paolo]
> >         Address Eric's comments:
> >         - Check has_foo before using foo.
> >         - Remove blank line between comments and definition in schema.
> >         - Use PRId64 instead of %ld.
> >         - Merge short lines.
> > 
> > v2: Add Eric's rev-by in patches 2, 4, 5.
> >     01: Refering -> referring in commit message. [Eric]
> >         Recurse to "file" for sensible "zero" flag. [Paolo]
> >     12: New. Make MapEntry a QAPI struct. [Paolo, Markus]
> > 
> > Original cover letter
> > ---------------------
> > 
> > I stumbled upon this when looking at external bitmap formats.
> > 
> > Current "qemu-img map" command only displays filename if the data is allocated
> > in bs (bs->file) itself, or in the backing chain. Otherwise, it displays an
> > unfriendly error message:
> > 
> >     $ qemu-img create -f vmdk -o subformat=monolithicFlat /tmp/test.vmdk 1G
> > 
> >     $ qemu-img map /tmp/test.vmdk
> >     Offset          Length          Mapped to       File
> >     qemu-img: File contains external, encrypted or compressed clusters.
> > 
> > This can be improved. This series extends the .bdrv_co_get_block_status
> > callback, to let block driver return the BDS of file; then updates all driver
> > to implement it; and lastly, it changes qemu-img to use this information in
> > "map" command:
> > 
> > 
> >     $ qemu-img map /tmp/test.vmdk
> >     Offset          Length          Mapped to       File
> >     0               0x40000000      0               /tmp/test-flat.vmdk
> > 
> >     $ qemu-img map --output json /tmp/test.vmdk
> >     [{"length": 1073741824, "start": 0, "zero": false, "offset": 0, "depth": 0,
> >       "file": "/tmp/test-flat.vmdk", "data": true}
> >     ]
> > 
> > Fam Zheng (14):
> >   block: Add "file" output parameter to block status query functions
> >   qcow: Assign bs->file->bs to file in qcow_co_get_block_status
> >   qcow2: Assign bs->file->bs to file in qcow2_co_get_block_status
> 
> Minor comment: I'd swap these two patches (2 and 3). Patch 1 breaks test
> 102, patch 3 fixes it again. It would be better to break it for as short
> a time as possible.
> 
> Alternatively, in order not to break 102 at all, patch 1 would need to
> leave the "if (bs->file &&" part of bdrv_co_get_block_status()
> (@@ -1544,13 +1550,14 @@) as-is and change it only after the format
> block drivers set *file.

Yes, this is better.

Fam