Re: [Qemu-devel] [PATCH v4 16/21] qemu-img: add a "map" subcommand

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

On 08/29/2013 08:00 AM, Paolo Bonzini wrote:
> This command dumps the metadata of an entire chain, in either tabular or JSON
> format.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-img-cmds.hx |   6 ++
>  qemu-img.c       | 191 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 197 insertions(+)
> 

> +++ b/qemu-img-cmds.hx
> @@ -45,6 +45,12 @@ STEXI
>  @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
>  ETEXI
>  
> +DEF("map", img_map,
> +    "map [-f fmt] [--output=ofmt] filename")
> +STEXI
> +@item map [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
> +ETEXI
> +

Should the matching changes to qemu.texi (currently in 17/21) be hoisted
into this patch?

> +    case OFORMAT_HUMAN:
> +        if ((e->flags & BDRV_BLOCK_DATA) &&
> +            !(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
> +            error_report("File contains external, encrypted or compressed clusters.");
> +            exit(1);
> +        }
> +        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
> +            printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",

printf("%#x", 0) prints 0, not 0x0.  The automatic 0x is only for
non-zero values; do we really want 0 to look different in the output
from all other values?  Let's see...

> +                   e->start, e->length, e->offset, e->bs->filename);

e->length is always non-zero.  For a qcow2 file, e->offset will never be
0 (the first raw cluster is always after the metadata).  But for a raw
file, the file starts at 0, so e->offset could be 0 there - is your new
'map' subcommand a useful way to probe where the holes are in a sparse
raw file?  But e->start will cover at most one entry starting at 0 (none
if the logical data is unallocated at the start of the file, since the
human output elides those blocks); and indeed, your example in 17/21
shows the special casing:

+@example
+Offset          Length          Mapped to       File
+0               0x20000         0x50000         /tmp/overlay.qcow2
+0x100000        0x10000         0x95380000      /tmp/backing.qcow2

I would have written "0x%-16"PRIx64 instead of "%#-16"PRIx64, but I can
live with your version.

> +    case OFORMAT_JSON:
> +        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": %d,"
> +               " \"zero\": %s, \"data\": %s",
> +               (e->start == 0 ? "[" : ",\n"),

Here, e->start==0 will always be present, even when unallocated (since
you don't elide any blocks, and the logical data always starts at 0).

> +
> +    if (output_format == OFORMAT_HUMAN) {
> +        printf("%-16s%-16s%-16s%s\n", "Offset", "Length", "Mapped to", "File");

Are we ever planning on marking up qemu-img for translation?  But this
patch need not worry about it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]