diff mbox

[v2,2/2] vmdk: implment bdrv_get_specific_info

Message ID 1381480281-14607-3-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Oct. 11, 2013, 8:31 a.m. UTC
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(-)

Comments

Eric Blake Oct. 11, 2013, 11:52 a.m. UTC | #1
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.
Fam Zheng Oct. 11, 2013, 12:07 p.m. UTC | #2
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
>
Max Reitz Oct. 11, 2013, 12:27 p.m. UTC | #3
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
diff mbox

Patch

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