[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct |
Date: |
Wed, 25 Nov 2015 08:36:29 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
- Re: [Qemu-devel] [PATCH v2 07/14] qed: Assign bs->file->bs to file in bdrv_qed_co_get_block_status, (continued)
- [Qemu-devel] [PATCH v2 08/14] sheepdog: Assign bs to file in sd_co_get_block_status, Fam Zheng, 2015/11/25
- [Qemu-devel] [PATCH v2 09/14] vdi: Assign bs->file->bs to file in vdi_co_get_block_status, Fam Zheng, 2015/11/25
- [Qemu-devel] [PATCH v2 10/14] vpc: Assign bs->file->bs to file in vpc_co_get_block_status, Fam Zheng, 2015/11/25
- [Qemu-devel] [PATCH v2 11/14] vmdk: Return extent's file in bdrv_get_block_status, Fam Zheng, 2015/11/25
- [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct, Fam Zheng, 2015/11/25
- [Qemu-devel] [PATCH v2 13/14] qemu-img: Use QAPI visitor to generate JSON, Fam Zheng, 2015/11/25
- [Qemu-devel] [PATCH v2 14/14] iotests: Add "qemu-img map" test for VMDK extents, Fam Zheng, 2015/11/25