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 <f...@redhat.com>
> > ---
> > 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
>
