From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1oG7-0005d4-KD for qemu-devel@nongnu.org; Wed, 25 Nov 2015 23:34:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1oG6-0006lq-F0 for qemu-devel@nongnu.org; Wed, 25 Nov 2015 23:34:15 -0500 Date: Thu, 26 Nov 2015 12:34:05 +0800 From: Fam Zheng Message-ID: <20151126043405.GE14630@ad.usersys.redhat.com> References: <1448437153-27090-1-git-send-email-famz@redhat.com> <1448437153-27090-13-git-send-email-famz@redhat.com> <5655D57D.8050400@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5655D57D.8050400@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , Peter Lieven , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini 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 > > --- > > 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 >