[Qemu-devel] [PATCH v2 0/2] vmdk: Implement bdrv_get_specific_info

[Qemu-devel] [PATCH v2 0/2] vmdk: Implement bdrv_get_specific_info

[Fam Zheng]

This is based on "[PATCH v3 0/2] vmdk: convert error reporting".

The new information looks like:

    image: /tmp/foo.vmdk
    file format: vmdk
    virtual size: 100G (107374182400 bytes)
    disk size: 4.0K
    Format specific information:
        cid: 0
        create_type: twoGbMaxExtentFlat
        parent cid: 0
        extents:
            [0]:
                virtual size: 2147483648
                filename: /tmp/foo-f001.vmdk
                format: FLAT
            [1]:
                virtual size: 2147483648
                filename: /tmp/foo-f002.vmdk
                format: FLAT
            [2]:
                virtual size: 2147483648
                filename: /tmp/foo-f003.vmdk
                format: FLAT
            [3]:                                           ...


Fam Zheng (2):
  qapi: Add optional field 'compressed' to ImageInfo
  vmdk: implment bdrv_get_specific_info

 block/vmdk.c               | 57 +++++++++++++++++++++++++++++++++++++++++++++-
 qapi-schema.json           | 28 +++++++++++++++++++++--
 tests/qemu-iotests/059     |  2 +-
 tests/qemu-iotests/059.out |  5 ++--
 4 files changed, 85 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] qapi: Add optional field 'compressed' to ImageInfo

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi-schema.json | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index a1a81a4..b187fdb 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -256,6 +256,8 @@
 #
 # @encrypted: #optional true if the image is encrypted
 #
+# @compressed: #optional true if the image is compressed (Since 1.7)
+#
 # @backing-filename: #optional name of the backing file
 #
 # @full-backing-filename: #optional full path of the backing file
@@ -276,7 +278,7 @@
 { 'type': 'ImageInfo',
   'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
            '*actual-size': 'int', 'virtual-size': 'int',
-           '*cluster-size': 'int', '*encrypted': 'bool',
+           '*cluster-size': 'int', '*encrypted': 'bool', '*compressed': 'bool',
            '*backing-filename': 'str', '*full-backing-filename': 'str',
            '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
            '*backing-image': 'ImageInfo',
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] vmdk: implment bdrv_get_specific_info

[Fam Zheng]

Implement .bdrv_get_specific_info to return the extent information.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c               | 57 +++++++++++++++++++++++++++++++++++++++++++++-
 qapi-schema.json           | 24 ++++++++++++++++++-
 tests/qemu-iotests/059     |  2 +-
 tests/qemu-iotests/059.out |  5 ++--
 4 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 90340eb..b4d5419 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -106,17 +106,20 @@ typedef struct VmdkExtent {
     uint32_t l2_cache_counts[L2_CACHE_SIZE];
 
     int64_t cluster_sectors;
+    char *type;
 } VmdkExtent;
 
 typedef struct BDRVVmdkState {
     CoMutex lock;
     uint64_t desc_offset;
     bool cid_updated;
+    uint32_t cid;
     uint32_t parent_cid;
     int num_extents;
     /* Extent array with num_extents entries, ascend ordered by address */
     VmdkExtent *extents;
     Error *migration_blocker;
+    char *create_type;
 } BDRVVmdkState;
 
 typedef struct VmdkMetaData {
@@ -215,6 +218,7 @@ static void vmdk_free_extents(BlockDriverState *bs)
         g_free(e->l1_table);
         g_free(e->l2_cache);
         g_free(e->l1_backup_table);
+        g_free(e->type);
         if (e->file != bs->file) {
             bdrv_unref(e->file);
         }
@@ -711,6 +715,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     int64_t flat_offset;
     char extent_path[PATH_MAX];
     BlockDriverState *extent_file;
+    BDRVVmdkState *s = bs->opaque;
+    VmdkExtent *extent;
 
     while (*p) {
         /* parse extent line:
@@ -751,7 +757,6 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         /* save to extents array */
         if (!strcmp(type, "FLAT") || !strcmp(type, "VMFS")) {
             /* FLAT extent */
-            VmdkExtent *extent;
 
             ret = vmdk_add_extent(bs, extent_file, true, sectors,
                             0, 0, 0, 0, 0, &extent, errp);
@@ -766,10 +771,12 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                 bdrv_unref(extent_file);
                 return ret;
             }
+            extent = &s->extents[s->num_extents - 1];
         } else {
             error_setg(errp, "Unsupported extent type '%s'", type);
             return -ENOTSUP;
         }
+        extent->type = g_strdup(type);
 next_line:
         /* move to next line */
         while (*p && *p != '\n') {
@@ -814,6 +821,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags,
         ret = -ENOTSUP;
         goto exit;
     }
+    s->create_type = g_strdup(ct);
     s->desc_offset = 0;
     ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
 exit:
@@ -1763,6 +1771,7 @@ static void vmdk_close(BlockDriverState *bs)
     BDRVVmdkState *s = bs->opaque;
 
     vmdk_free_extents(bs);
+    g_free(s->create_type);
 
     migrate_del_blocker(s->migration_blocker);
     error_free(s->migration_blocker);
@@ -1824,6 +1833,51 @@ static int vmdk_has_zero_init(BlockDriverState *bs)
     return 1;
 }
 
+static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
+{
+    ImageInfo *info = g_new0(ImageInfo, 1);
+
+    *info = (ImageInfo){
+        .filename         = g_strdup(extent->file->filename),
+        .format           = g_strdup(extent->type),
+        .virtual_size     = extent->sectors * BDRV_SECTOR_SIZE,
+        .compressed       = extent->compressed,
+        .has_compressed   = extent->compressed,
+        .cluster_size     = extent->cluster_sectors * BDRV_SECTOR_SIZE,
+        .has_cluster_size = !extent->flat,
+    };
+
+    return info;
+}
+
+static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
+{
+    int i;
+    BDRVVmdkState *s = bs->opaque;
+    ImageInfoSpecific *spec_info = g_new0(ImageInfoSpecific, 1);
+    ImageInfoList **next;
+
+    *spec_info = (ImageInfoSpecific){
+        .kind = IMAGE_INFO_SPECIFIC_KIND_VMDK,
+        .vmdk = g_new0(ImageInfoSpecificVmdk, 1),
+    };
+
+    *spec_info->vmdk = (ImageInfoSpecificVmdk) {
+        .create_type = g_strdup(s->create_type),
+        .cid = s->cid,
+    };
+
+    next = &spec_info->vmdk->extents;
+    for (i = 0; i < s->num_extents; i++) {
+        *next = g_new0(ImageInfoList, 1);
+        (*next)->value = vmdk_get_extent_info(&s->extents[i]);
+        (*next)->next = NULL;
+        next = &(*next)->next;
+    }
+
+    return spec_info;
+}
+
 static QEMUOptionParameter vmdk_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1876,6 +1930,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_co_get_block_status     = vmdk_co_get_block_status,
     .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
     .bdrv_has_zero_init           = vmdk_has_zero_init,
+    .bdrv_get_specific_info       = vmdk_get_specific_info,
 
     .create_options               = vmdk_create_options,
 };
diff --git a/qapi-schema.json b/qapi-schema.json
index b187fdb..093f28b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -225,6 +225,27 @@
   } }
 
 ##
+# @ImageInfoSpecificVmdk:
+#
+# @create_type: The create type of VMDK image
+#
+# @cid: Content id of image
+#
+# @parent-cid: Parent VMDK image's cid
+#
+# @extents: List of extent files
+#
+# Since: 1.7
+##
+{ 'type': 'ImageInfoSpecificVmdk',
+  'data': {
+      'create_type': 'str',
+      'cid': 'int',
+      'parent-cid': 'int',
+      'extents': ['ImageInfo']
+  } }
+
+##
 # @ImageInfoSpecific:
 #
 # A discriminated record of image format specific information structures.
@@ -234,7 +255,8 @@
 
 { 'union': 'ImageInfoSpecific',
   'data': {
-      'qcow2': 'ImageInfoSpecificQCow2'
+      'qcow2': 'ImageInfoSpecificQCow2',
+      'vmdk': 'ImageInfoSpecificVmdk'
   } }
 
 ##
diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059
index 26d4538..36103e1 100755
--- a/tests/qemu-iotests/059
+++ b/tests/qemu-iotests/059
@@ -69,7 +69,7 @@ poke_file "$TEST_IMG" "$grain_table_size_offset" "\x01\x00\x00\x00"
 echo "=== Testing monolithicFlat creation and opening ==="
 echo
 IMGOPTS="subformat=monolithicFlat" _make_test_img 2G
-$QEMU_IMG info $TEST_IMG | _filter_testdir
+_img_info
 
 echo
 echo "=== Testing monolithicFlat with zeroed_grain ==="
diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 2b29ca9..5829794 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -18,10 +18,9 @@ no file open, try 'help open'
 === Testing monolithicFlat creation and opening ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
-image: TEST_DIR/t.vmdk
-file format: vmdk
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
 virtual size: 2.0G (2147483648 bytes)
-disk size: 4.0K
 
 === Testing monolithicFlat with zeroed_grain ===
 qemu-img: TEST_DIR/t.IMGFMT: Flat image can't enable zeroed grain
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/2] vmdk: Implement bdrv_get_specific_info

[Kevin Wolf]

Am 11.10.2013 um 10:31 hat Fam Zheng geschrieben:
> This is based on "[PATCH v3 0/2] vmdk: convert error reporting".

I have that series in the block branch now, but it still doesn't apply.
Can you rebase?

Kevin

Re: [Qemu-devel] [PATCH v2 2/2] vmdk: implment bdrv_get_specific_info

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

On 10/11/2013 02:31 AM, Fam Zheng wrote:
> Implement .bdrv_get_specific_info to return the extent information.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

> +
> +    *spec_info->vmdk = (ImageInfoSpecificVmdk) {
> +        .create_type = g_strdup(s->create_type),
> +        .cid = s->cid,
> +    };
> +

>  
>  ##
> +# @ImageInfoSpecificVmdk:
> +#
> +# @create_type: The create type of VMDK image

Is it worth making this an enum type rather than an open-coded string?
But that's not a show-stopper to me.

> +#
> +# @cid: Content id of image
> +#
> +# @parent-cid: Parent VMDK image's cid
> +#
> +# @extents: List of extent files
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'ImageInfoSpecificVmdk',
> +  'data': {
> +      'create_type': 'str',
> +      'cid': 'int',
> +      'parent-cid': 'int',
> +      'extents': ['ImageInfo']
> +  } }

Both patches look fine from the QMP point of view; I didn't closely
review the matching C code for accuracy though.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] vmdk: implment bdrv_get_specific_info

[Fam Zheng]

On Fri, 10/11 05:52, Eric Blake wrote:
> On 10/11/2013 02:31 AM, Fam Zheng wrote:
> > Implement .bdrv_get_specific_info to return the extent information.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> 
> > +
> > +    *spec_info->vmdk = (ImageInfoSpecificVmdk) {
> > +        .create_type = g_strdup(s->create_type),
> > +        .cid = s->cid,
> > +    };
> > +
> 
> >  
> >  ##
> > +# @ImageInfoSpecificVmdk:
> > +#
> > +# @create_type: The create type of VMDK image
> 
> Is it worth making this an enum type rather than an open-coded string?
> But that's not a show-stopper to me.

For now I think a string is good enough, it's only used to display with
qemu-img info, string type saves converting in opening code, as well as
repeating type names in multiple places.

We can convert them in the future if there comes more uses of the types.

Fam

> 
> > +#
> > +# @cid: Content id of image
> > +#
> > +# @parent-cid: Parent VMDK image's cid
> > +#
> > +# @extents: List of extent files
> > +#
> > +# Since: 1.7
> > +##
> > +{ 'type': 'ImageInfoSpecificVmdk',
> > +  'data': {
> > +      'create_type': 'str',
> > +      'cid': 'int',
> > +      'parent-cid': 'int',
> > +      'extents': ['ImageInfo']
> > +  } }
> 
> Both patches look fine from the QMP point of view; I didn't closely
> review the matching C code for accuracy though.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v2 2/2] vmdk: implment bdrv_get_specific_info

[Max Reitz]

On 2013-10-11 14:07, Fam Zheng wrote:
> On Fri, 10/11 05:52, Eric Blake wrote:
>> On 10/11/2013 02:31 AM, Fam Zheng wrote:
>>> Implement .bdrv_get_specific_info to return the extent information.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>> +
>>> +    *spec_info->vmdk = (ImageInfoSpecificVmdk) {
>>> +        .create_type = g_strdup(s->create_type),
>>> +        .cid = s->cid,
>>> +    };
>>> +
>>>   
>>>   ##
>>> +# @ImageInfoSpecificVmdk:
>>> +#
>>> +# @create_type: The create type of VMDK image
>> Is it worth making this an enum type rather than an open-coded string?
>> But that's not a show-stopper to me.
> For now I think a string is good enough, it's only used to display with
> qemu-img info, string type saves converting in opening code, as well as
> repeating type names in multiple places.

Actually, it can be queried through QMP (query-block) as well. But I 
personally don't oppose using a string here.

Max