From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40732) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1c7e-0005PT-An for qemu-devel@nongnu.org; Wed, 25 Nov 2015 10:36:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1c7d-0002f6-5L for qemu-devel@nongnu.org; Wed, 25 Nov 2015 10:36:42 -0500 References: <1448437153-27090-1-git-send-email-famz@redhat.com> <1448437153-27090-13-git-send-email-famz@redhat.com> From: Eric Blake Message-ID: <5655D57D.8050400@redhat.com> Date: Wed, 25 Nov 2015 08:36:29 -0700 MIME-Version: 1.0 In-Reply-To: <1448437153-27090-13-git-send-email-famz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="E90gXSbf5vc47k6HXlUbQQMf1fsTpMu0d" 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: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Jeff Cody , Peter Lieven , Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --E90gXSbf5vc47k6HXlUbQQMf1fsTpMu0d Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > Signed-off-by: Fam Zheng > --- > qapi/block-core.json | 28 ++++++++++++++++++++++++++++ > qemu-img.c | 48 ++++++++++++++++++++++----------------------= ---- > 2 files changed, 50 insertions(+), 26 deletions(-) >=20 > +## > + > +{ '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. >=20 > - if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) =3D=3D 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", \"dep= th\": %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 *b= s, int64_t sector_num, > =20 > e->start =3D sector_num * BDRV_SECTOR_SIZE; > e->length =3D nb_sectors * BDRV_SECTOR_SIZE; > - e->flags =3D ret & ~BDRV_BLOCK_OFFSET_MASK; > + e->data =3D !!(ret & BDRV_BLOCK_DATA); > + e->zero =3D !!(ret & BDRV_BLOCK_ZERO); > e->offset =3D ret & BDRV_BLOCK_OFFSET_MASK; > + e->has_offset =3D !!(ret & BDRV_BLOCK_OFFSET_VALID); [1]... offset might be garbage if has_offset is false. > e->depth =3D depth; > - e->bs =3D bs; > + if (e->has_offset) { > + e->has_filename =3D true; > + e->filename =3D 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)? > =20 > @@ -2307,9 +2301,11 @@ static int img_map(int argc, char **argv) > goto out; > } > =20 > - if (curr.length !=3D 0 && curr.flags =3D=3D next.flags && > + if (curr.length !=3D 0 && curr.zero =3D=3D next.zero && > + curr.data =3D=3D next.data && > curr.depth =3D=3D next.depth && > - ((curr.flags & BDRV_BLOCK_OFFSET_VALID) =3D=3D 0 || > + !strcmp(curr.filename, next.filename) && Is this strcmp valid even when !has_filename? > + (!curr.has_offset || > curr.offset + curr.length =3D=3D next.offset)) { > curr.length +=3D next.length; > continue; >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --E90gXSbf5vc47k6HXlUbQQMf1fsTpMu0d Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWVdV9AAoJEKeha0olJ0Nq+GAH/jkDFlOaptLVT+ysl3jgewi+ md+yDLiuCdh5h09ieNXczz5GAKXcWvjfnue9EkQKzyRRL4dsuUsqvzCzaJ/Sl1Mn kqFQPclDKWg7Fb2YnUsi5tXPY6UsNu5xwAEoiHIN80+p/rn0cfLDBU3RbDZK8pL6 T4WfcLmkJsaY2pMKbHsJa+rH7nWCLvXC2BxyNNLhcu6awErOiyqIAr/YW8pKTJcl OYt68MY9ufDNfqs5TRajOFxUOeLQw3NCzJ8OlvjE8xx9eM2qUucRnIvaFuKfluX7 5zAeYTYWD68VeX5ejXtlmUGwGc54ez2vTa+cq68ojyLFN01bLiiIiUD9vv80asU= =REAN -----END PGP SIGNATURE----- --E90gXSbf5vc47k6HXlUbQQMf1fsTpMu0d--