Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct
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
>
Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct
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.
> + '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 12/14] qemu-img: Make MapEntry a QAPI struct
On 25/11/2015 08:39, Fam Zheng wrote:
> */
> -if (next &&
> -(next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) !=
> BDRV_BLOCK_DATA) {
> -next->flags &= ~BDRV_BLOCK_DATA;
> -next->flags |= BDRV_BLOCK_ZERO;
> +if (next && !next->data) {
> +next->zero = true;
before after
0 ZEROZERO
DATADATADATA
DATA|ZERO ZERODATA|ZERO
ZEROZEROZERO
This would not coalesce 0 with DATA|ZERO.
I think you need to do exactly as in the older code: test (!next->data
|| next->zero), and clear next->data.
Paolo
[Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct
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(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a07b13f..db62bda 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -186,6 +186,34 @@
'*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
##
+# @MapEntry:
+#
+# Mapping information from a virtual block range to a host file range
+#
+# @start: the start byte of the mapped virtual range
+#
+# @length: the number of bytes of the mapped virtual range
+#
+# @data: whether the mapped range has data
+#
+# @zero: whether the virtual blocks are zeroed
+#
+# @depth: the depth of the mapping
+#
+# @offset: #optional the offset in file that the virtual sectors are mapped to
+#
+# @filename: #optional filename that is referred to by @offset
+#
+# Since: 2.6
+#
+##
+
+{ 'struct': 'MapEntry',
+ 'data': {'start': 'int', 'length': 'int', 'data': 'bool',
+ 'zero': 'bool', 'depth': 'int', '*offset': 'int',
+ '*filename': 'str' } }
+
+##
# @BlockdevCacheInfo
#
# Cache mode information for a block device
diff --git a/qemu-img.c b/qemu-img.c
index 7954242..0ccb396 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2133,47 +2133,36 @@ static int img_info(int argc, char **argv)
return 0;
}
-
-typedef struct MapEntry {
-int flags;
-int depth;
-int64_t start;
-int64_t length;
-int64_t offset;
-BlockDriverState *bs;
-} MapEntry;
-
static void dump_map_entry(OutputFormat output_format, MapEntry *e,
MapEntry *next)
{
switch (output_format) {
case OFORMAT_HUMAN:
-if ((e->flags & BDRV_BLOCK_DATA) &&
-!(e->flags & BDRV_BLOCK_OFFSET_VALID)) {
+if (e->data && !e->has_offset) {
error_report("File contains external, encrypted or compressed
clusters.");
exit(1);
}
-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 format ignores the distinction between 0, ZERO and ZERO|DATA.
* Modify the flags here to allow more coalescing.
*/
-if (next &&
-(next->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) !=
BDRV_BLOCK_DATA) {
-next->flags &= ~BDRV_BLOCK_DATA;
-next->flags |= BDRV_BLOCK_ZERO;
+if (next && !next->data) {
+next->zero = true;
}
break;
case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64", \"depth\":
%d,"
+printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
+ " \"depth\": %ld,"
" \"zero\": %s, \"data\": %s",
(e->start == 0 ? "[" : ",\n"),
e->start, e->length, e->depth,
- (e->flags & BDRV_BLOCK_ZERO) ? "true" : "false",
- (e->flags & BDRV_BLOCK_DATA) ? "true" : "false");
-if (e->flags & BDRV_BLOCK_OFFSET_VALID) {
+ e->zero ? "true" : "false",
+ e->data ? "true" : "false");
+if (e->has_offset) {
printf(", \"offset\": %"PRId64"", e->offset);
}
putchar('}');
@@ -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);
e->depth = depth;
-e->bs = bs;
+if (e->has_offset) {
+e->has_filename = true;
+e->filename = bs->filename;
+}
return 0;
}
@@ -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) &&
+(!curr.has_offset ||
curr.offset + curr.length == next.offset)) {
curr.length += next.length;
continue;
--
2.4.3
