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

[Fam Zheng]

On Wed, 11/25 08:36, Eric Blake wrote:
> 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.

This is copied from ImageInfo above. Removing.

> 
> > +  '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]

Will add check.

> 
> >      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.

Yes.

> 
> >                 " \"zero\": %s, \"data\": %s",
> 
> Worth joining the two short lines into one?

OK.

> 
> > @@ -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)?

It's an array field:

    struct BlockDriverState {
        ...
        char filename[PATH_MAX];

So I think this is OK.

> 
> >  
> > @@ -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?

No, will check it.

Thanks for reviewing!

Fam

> 
> > +            (!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
>