Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

[Eric Blake]

On 11/25/2015 12:39 AM, Fam Zheng wrote:
> The "flags" bit mask is expanded to two booleans, "data" and "zero";
> "bs" is replaced with "filename" string.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  qapi/block-core.json | 28 ++++++++++++++++++++++++++++
>  qemu-img.c           | 48 ++++++++++++++++++++++--------------------------
>  2 files changed, 50 insertions(+), 26 deletions(-)
> 

> +##
> +
> +{ 'struct': 'MapEntry',

Blank line is not typical here.

> +  'data': {'start': 'int', 'length': 'int', 'data': 'bool',
> +           'zero': 'bool', 'depth': 'int', '*offset': 'int',
> +           '*filename': 'str' } }

Struct looks fine.

> 
> -        if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == 
> BDRV_BLOCK_DATA) {
> +        if (e->data && !e->zero) {
>              printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
> -                   e->start, e->length, e->offset, e->bs->filename);
> +                   e->start, e->length, e->offset,
> +                   e->has_filename ? e->filename : "");
>          }

This blindly prints e->offset, even though...[1]

>      case OFORMAT_JSON:
> -        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\": 
> %d,"
> +        printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
> +               " \"depth\": %ld,"

%ld is wrong; it needs to be PRId64.

>                 " \"zero\": %s, \"data\": %s",

Worth joining the two short lines into one?

> @@ -2219,10 +2208,15 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t sector_num,
>  
>      e->start = sector_num * BDRV_SECTOR_SIZE;
>      e->length = nb_sectors * BDRV_SECTOR_SIZE;
> -    e->flags = ret & ~BDRV_BLOCK_OFFSET_MASK;
> +    e->data = !!(ret & BDRV_BLOCK_DATA);
> +    e->zero = !!(ret & BDRV_BLOCK_ZERO);
>      e->offset = ret & BDRV_BLOCK_OFFSET_MASK;
> +    e->has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);

[1]... offset might be garbage if has_offset is false.

>      e->depth = depth;
> -    e->bs = bs;
> +    if (e->has_offset) {
> +        e->has_filename = true;
> +        e->filename = bs->filename;
> +    }
>      return 0;
>  }

Are we guaranteed that bs->filename is non-NULL when offset is set?  Or
does this need to be if (e->has_offset && bs->filename)?

>  
> @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv)
>              goto out;
>          }
>  
> -        if (curr.length != 0 && curr.flags == next.flags &&
> +        if (curr.length != 0 && curr.zero == next.zero &&
> +            curr.data == next.data &&
>              curr.depth == next.depth &&
> -            ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
> +            !strcmp(curr.filename, next.filename) &&

Is this strcmp valid even when !has_filename?

> +            (!curr.has_offset ||
>               curr.offset + curr.length == next.offset)) {
>              curr.length += next.length;
>              continue;
> 

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

signature.asc
Description: OpenPGP digital signature