Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

2015-11-25 Thread Eric Blake 
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

2015-11-25 Thread Paolo Bonzini 


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

Re: [Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

2015-11-25 Thread Fam Zheng 
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
>

[Qemu-devel] [PATCH v2 12/14] qemu-img: Make MapEntry a QAPI struct

2015-11-24 Thread Fam Zheng 
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