vmdk: Implement blockdev-create
v3 cover vmdk: Implement blockdev-create
I picked up the patch series from Fam and rebased it to current master (which involved a major rework on the test case) and tried to address Markus' review comments for v2. I did not do any further review of the actual code, but it passes the tests, so I guess having it in tree is better than continuing to let it bitrot.
Fam Zheng (3):
vmdk: Refactor vmdk_create_extent
vmdk: Implement .bdrv_co_create callback
iotests: Filter cid numbers in VMDK extent info
Kevin Wolf (1):
iotests: Add VMDK tests for blockdev-create
qapi/block-core.json | 70 +++++ qapi/qapi-schema.json | 1 + block/vmdk.c | 519 ++++++++++++++++++++++--------- tests/qemu-iotests/237 | 201 ++++++++++++ tests/qemu-iotests/237.out | 309 ++++++++++++++++++ tests/qemu-iotests/common.filter | 1 + tests/qemu-iotests/group | 1 + tests/qemu-iotests/iotests.py | 1 + 8 files changed, 951 insertions(+), 152 deletions(-) create mode 100755 tests/qemu-iotests/237 create mode 100644 tests/qemu-iotests/237.out
Signature · 1 line
2.19.2
PATCH v3 1/4 vmdk: Refactor vmdk_create_extent
From: Fam Zheng <[email protected]>
The extracted vmdk_init_extent takes a BlockBackend object and initializes the format metadata. It is the common part between "qemu-img create" and "blockdev-create".
Add a "BlockBackend *pbb" parameter to vmdk_create_extent, to return the opened BB to the caller in the next patch.
--- block/vmdk.c | 69 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 26 deletions(-)
block/vmdk.c +43 -26
@@ -1741,35 +1741,17 @@ static int coroutine_fn vmdk_co_pwrite_zeroes(BlockDriverState *bs,return ret; } -static int vmdk_create_extent (const char *filename,int64_t filesize, -bool flat,bool compress,bool zeroed_grain, -QemuOpts *opts,Error **errp) +static int vmdk_init_extent (BlockBackend *blk, +int64_t filesize,bool flat, +bool compress,bool zeroed_grain, +Error **errp) {int ret, i; -BlockBackend *blk =NULL ;VMDK4Header header; -Error *local_err =NULL ;uint32_t tmp, magic, grains, gd_sectors, gt_size, gt_count;uint32_t *gd_buf =NULL ;int gd_buf_size; - ret =bdrv_create_file (filename, opts, &local_err); -if (ret <0 ) { -error_propagate (errp, local_err); - goto exit; - } - - blk =blk_new_open (filename,NULL ,NULL , -BDRV_O_RDWR |BDRV_O_RESIZE |BDRV_O_PROTOCOL , - &local_err); -if (blk ==NULL ) { -error_propagate (errp, local_err); - ret = -EIO ; - goto exit; - } - -blk_set_allow_write_beyond_eof (blk, true); -if (flat) { ret =blk_truncate (blk, filesize,PREALLOC_MODE_OFF , errp); goto exit;
@@ -1863,15 +1845,50 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, gd_buf, gd_buf_size, 0);if (ret <0 ) {error_setg (errp,QERR_IO_ERROR ); - goto exit; } ret =0 ; +exit: +g_free (gd_buf); +return ret; +} + +static int vmdk_create_extent (const char *filename,int64_t filesize, +bool flat,bool compress,bool zeroed_grain, +BlockBackend **pbb, +QemuOpts *opts,Error **errp) +{ +int ret; +BlockBackend *blk =NULL ; +Error *local_err =NULL ; + + ret =bdrv_create_file (filename, opts, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto exit; + } + + blk =blk_new_open (filename,NULL ,NULL , +BDRV_O_RDWR |BDRV_O_RESIZE |BDRV_O_PROTOCOL , + &local_err); +if (blk ==NULL ) { +error_propagate (errp, local_err); + ret = -EIO ; + goto exit; + } + +blk_set_allow_write_beyond_eof (blk, true); + + ret =vmdk_init_extent (blk, filesize, flat, compress, zeroed_grain, errp); exit:if (blk) { -blk_unref (blk); +if (pbb) { + *pbb = blk; + }else { +blk_unref (blk); + blk =NULL ; + } } -g_free (gd_buf);return ret; }
@@ -2094,7 +2111,7 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *optssnprintf (ext_filename,PATH_MAX ,"%s%s" , path, desc_filename);if (vmdk_create_extent (ext_filename, size, - flat, compress, zeroed_grain, opts, errp)) { + flat, compress, zeroed_grain,NULL , opts, errp)) { ret = -EINVAL ; goto exit; }
Signature · 1 line
2.19.2
Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
Quoted 8 lines · Kevin Wolf
From: Fam Zheng <[email protected]>
The extracted
vmdk_init_extenttakes a BlockBackend object and initializes the format metadata. It is the common part between "qemu-img create" and "blockdev-create".Add a "BlockBackend *pbb" parameter to
vmdk_create_extent, to return the opened BB to the caller in the next patch.
I'd do this part in the next patch, when it has a user. Matter of taste.
Quoted 89 lines
Signed-off-by Fam Zheng <[email protected]>Signed-off-by Kevin Wolf <[email protected]>--- block/vmdk.c | 69 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 26 deletions(-)block/vmdk.c +34
-23@@ -1741,35 +1741,17 @@ static int coroutine_fn vmdk_co_pwrite_zeroes(BlockDriverState *bs,return ret; } -static int vmdk_create_extent (const char *filename,int64_t filesize, -bool flat,bool compress,bool zeroed_grain, -QemuOpts *opts,Error **errp) +static int vmdk_init_extent (BlockBackend *blk, +int64_t filesize,bool flat, +bool compress,bool zeroed_grain, +Error **errp) {int ret, i; -BlockBackend *blk =NULL ;VMDK4Header header; -Error *local_err =NULL ;uint32_t tmp, magic, grains, gd_sectors, gt_size, gt_count;uint32_t *gd_buf =NULL ;int gd_buf_size; - ret =bdrv_create_file (filename, opts, &local_err); -if (ret <0 ) { -error_propagate (errp, local_err); - goto exit; - } - - blk =blk_new_open (filename,NULL ,NULL , -BDRV_O_RDWR |BDRV_O_RESIZE |BDRV_O_PROTOCOL , - &local_err); -if (blk ==NULL ) { -error_propagate (errp, local_err); - ret = -EIO ; - goto exit; - } - -blk_set_allow_write_beyond_eof (blk, true); -if (flat) { ret =blk_truncate (blk, filesize,PREALLOC_MODE_OFF , errp); goto exit;@@ -1863,15 +1845,50 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, gd_buf, gd_buf_size, 0);if (ret <0 ) {error_setg (errp,QERR_IO_ERROR ); - goto exit; } ret =0 ; +exit: +g_free (gd_buf); +return ret; +} + +static int vmdk_create_extent (const char *filename,int64_t filesize, +bool flat,bool compress,bool zeroed_grain, +BlockBackend **pbb, +QemuOpts *opts,Error **errp) +{ +int ret; +BlockBackend *blk =NULL ; +Error *local_err =NULL ; + + ret =bdrv_create_file (filename, opts, &local_err); +if (ret <0 ) { +error_propagate (errp, local_err); + goto exit; + } + + blk =blk_new_open (filename,NULL ,NULL , +BDRV_O_RDWR |BDRV_O_RESIZE |BDRV_O_PROTOCOL , + &local_err); +if (blk ==NULL ) { +error_propagate (errp, local_err); + ret = -EIO ; + goto exit; + } + +blk_set_allow_write_beyond_eof (blk, true);
block/vmdk.c static int vmdk_create_extent(const char *filename, int64_t filesize, · line 1889
Quoted 10 lines
block/vmdk.c +7
-1+ + ret =vmdk_init_extent (blk, filesize, flat, compress, zeroed_grain, errp); exit:if (blk) { -blk_unref (blk); +if (pbb) { + *pbb = blk; + }else { +blk_unref (blk); + blk =NULL ;
Dead assignment. Matter of taste.
Quoted 7 lines
block/vmdk.c +1
-1+ } } -g_free (gd_buf);return ret; }
block/vmdk.c static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts · line 2117
Quoted 8 lines
block/vmdk.c +1
-1@@ -2094,7 +2111,7 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *optssnprintf (ext_filename,PATH_MAX ,"%s%s" , path, desc_filename);if (vmdk_create_extent (ext_filename, size, - flat, compress, zeroed_grain, opts, errp)) { + flat, compress, zeroed_grain,NULL , opts, errp)) { ret = -EINVAL ; goto exit; }
Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
Am 07.12.2018 um 07:40 hat Markus Armbruster geschrieben:
Quoted 8 lines
Quoted 6 lines · Kevin Wolf
From: Fam Zheng <[email protected]>
The extracted
vmdk_init_extenttakes a BlockBackend object and initializes the format metadata. It is the common part between "qemu-img create" and "blockdev-create".
Quoted 5 lines
Quoted 2 lines
Add a "BlockBackend *pbb" parameter to
vmdk_create_extent, to return the opened BB to the caller in the next patch.I'd do this part in the next patch, when it has a user. Matter of taste.
I kept Fam as the author of the patches (except for the test case which is effectively a rewrite with Fam's test as the spec), so I'd like to do only the minimal editing necessary to make the patches mergeable. If it's a matter of taste, I'll leave it as it is.
The same goes for code changes that are a matter of taste: If you _really_ care about taste in the vmdk code, feel free to send a follow-up and I'll merge it on top, but I'll leave Fam's patches alone as much as I can.
Quoted 1 line
Reviewed-by Markus Armbruster <[email protected]>
Thanks!
Kevin
Re: [Qemu-devel] [PATCH v3 1/4] vmdk: Refactor vmdk_create_extent
Quoted 12 lines · Kevin Wolf
Am 07.12.2018 um 07:40 hat Markus Armbruster geschrieben:
Quoted 11 lines
Quoted 8 lines · Kevin Wolf
From: Fam Zheng <[email protected]>
The extracted
vmdk_init_extenttakes a BlockBackend object and initializes the format metadata. It is the common part between "qemu-img create" and "blockdev-create".Add a "BlockBackend *pbb" parameter to
vmdk_create_extent, to return the opened BB to the caller in the next patch.
Quoted 7 lines
Quoted 2 lines
I'd do this part in the next patch, when it has a user. Matter of taste.
I kept Fam as the author of the patches (except for the test case which is effectively a rewrite with Fam's test as the spec), so I'd like to do only the minimal editing necessary to make the patches mergeable. If it's a matter of taste, I'll leave it as it is.
That's fine. Matter of taste means I'm stating mine without demanding you change the patch to conform to it.
Quoted 4 lines
The same goes for code changes that are a matter of taste: If you _really_ care about taste in the vmdk code, feel free to send a follow-up and I'll merge it on top, but I'll leave Fam's patches alone as much as I can.
Makes sense.
Quoted 5 lines
Quoted 1 line
Reviewed-by Markus Armbruster <[email protected]>Thanks!
Kevin
PATCH v3 2/4 vmdk: Implement .bdrv_co_create callback
In reply to Kevin WolfFrom: Fam Zheng <[email protected]>
This makes VMDK support blockdev-create. The implementation reuses the image creation code in vmdk_co_create_opts which now acceptes a callback pointer to "retrieve" BlockBackend pointers from the caller. This way we separate the logic between file/extent acquisition and initialization.
The QAPI command parameters are mostly the same as the old create_opts except the dropped legacy @compat6 switch, which is redundant with @hwversion.
--- qapi/block-core.json | 70 +++++++ qapi/qapi-schema.json | 1 + block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ 3 files changed, 396 insertions(+), 127 deletions(-)
qapi/block-core.json +70 -0
@@ -4021,6 +4021,75 @@ 'size': 'size', '*cluster-size' : 'size' } } +## +# @BlockdevVmdkSubformat: +# +# Subformat options for VMDK images +# +# @monolithicSparse: Single file image with sparse cluster allocation +# +# @monolithicFlat: Single flat data image and a descriptor file +# +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent +# files, in addition to a descriptor file +# +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent +# files, in addition to a descriptor file +# +# @streamOptimized: Single file image sparse cluster allocation, optimized +# for streaming over network. +# +# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkSubformat', + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse', + 'twoGbMaxExtentFlat', 'streamOptimized'] } + +## +# @BlockdevVmdkAdapterType: +# +# Adapter type info for VMDK images +# +# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkAdapterType', + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] } + +## +# @BlockdevCreateOptionsVmdk: +# +# Driver specific image creation options for VMDK. +# +# @file Where to store the new image file. This refers to the image +# file for monolithcSparse and streamOptimized format, or the +# descriptor file for other formats. +# @size Size of the virtual disk in bytes +# @extents Where to store the data extents. Required for monolithcflat, +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For +# monolithicflat, only one entry is required; for +# twoGbMaxExtent* formats, the number of entries required is +# calculated as extent_number = virtual_size / 2GB. +# @subformat The subformat of the VMDK image. Default: "monolithicsparse". +# @backing-file The path of backing file. Default: no backing file is used. +# @adapter-type The adapter type used to fill in the descriptor. Default: ide. +# @hwversion Hardware version. The meaningful options are "4" or "6". +# Defaulted to "4". +# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats. +# Default: false. +# +# Since: 4.0 +## +{ 'struct': 'BlockdevCreateOptionsVmdk', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*extents': ['BlockdevRef'], + '*subformat': 'BlockdevVmdkSubformat', + '*backing-file': 'str', + '*adapter-type': 'BlockdevVmdkAdapterType', + '*hwversion': 'str', + '*zeroed-grain': 'bool' } } + + ## # @SheepdogRedundancyType: #
@@ -4215,6 +4284,7 @@ 'ssh': 'BlockdevCreateOptionsSsh', 'vdi': 'BlockdevCreateOptionsVdi', 'vhdx': 'BlockdevCreateOptionsVhdx', + 'vmdk': 'BlockdevCreateOptionsVmdk', 'vpc': 'BlockdevCreateOptionsVpc' } }
qapi/qapi-schema.json +1 -0
@@ -66,6 +66,7 @@ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status 'CpuInfoMIPS', # PC, visible through query-cpu 'CpuInfoTricore', # PC, visible through query-cpu + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings 'QapiErrorClass', # all members, visible through errors 'UuidInfo', # UUID, visible through query-uuid 'X86CPURegister32', # all members, visible indirectly through qom-get
block/vmdk.c +325 -127
558 lines not on this page · raw message
Signature · 1 line
2.19.2
Re: [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
This is a reasonably careful review of the QAPI-related parts, but more of an eye-over for the remainder.
Quoted 40 lines · Kevin Wolf
From: Fam Zheng <[email protected]>
This makes VMDK support blockdev-create. The implementation reuses the image creation code in
vmdk_co_create_optswhich now acceptes a callback pointer to "retrieve" BlockBackend pointers from the caller. This way we separate the logic between file/extent acquisition and initialization.The QAPI command parameters are mostly the same as the old
create_optsexcept the dropped legacy @compat6 switch, which is redundant with @hwversion.Signed-off-by Fam Zheng <[email protected]>Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 70 +++++++ qapi/qapi-schema.json | 1 + block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ 3 files changed, 396 insertions(+), 127 deletions(-)qapi/block-core.json +13
-0@@ -4021,6 +4021,75 @@ 'size': 'size', '*cluster-size' : 'size' } } +## +# @BlockdevVmdkSubformat: +# +# Subformat options for VMDK images +# +# @monolithicSparse: Single file image with sparse cluster allocation +# +# @monolithicFlat: Single flat data image and a descriptor file +# +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent +# files, in addition to a descriptor file +# +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extent
qapi/block-core.json line 4046
Quoted 10 lines
qapi/block-core.json +10
-0+# files, in addition to a descriptor file +# +# @streamOptimized: Single file image sparse cluster allocation, optimized +# for streaming over network. +# +# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkSubformat', + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse', + 'twoGbMaxExtentFlat', 'streamOptimized'] }
Don't conform to the QAPI rules on names "to match VMDK spec spellings" (see qapi-schema.json below). We discussed this in review of v1.
PRO: matches the VMDK spec (whatever that's worth), keeps the code simple.
CON: the non-conforming names become part of the stable QMP interface, in the argument of command blockdev-create.
I don't like the CON, but I'm willing to tolerate it in the name of simplicity.
qapi/block-core.json line 4056
Quoted 10 lines
qapi/block-core.json +10
-0+ +## +# @BlockdevVmdkAdapterType: +# +# Adapter type info for VMDK images +# +# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkAdapterType', + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }
May I have hyphens in the composite nouns? Hmm, might be the way they are to match VMDK spec spellings or for backward compatibility. I guess we'll see below.
qapi/block-core.json line 4069
Quoted 13 lines
qapi/block-core.json +13
-0+ +## +# @BlockdevCreateOptionsVmdk: +# +# Driver specific image creation options for VMDK. +# +# @file Where to store the new image file. This refers to the image +# file for monolithcSparse and streamOptimized format, or the +# descriptor file for other formats. +# @size Size of the virtual disk in bytes +# @extents Where to store the data extents. Required for monolithcflat, +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For +# monolithicflat, only one entry is required; for
monolithicFlat
qapi/block-core.json line 4071
Quoted 2 lines
qapi/block-core.json +2
-0+# twoGbMaxExtent* formats, the number of entries required is +# calculated as extent_number = virtual_size / 2GB.
Is it okay to supply more entries than required, or do I have to supply exactly the right number?
qapi/block-core.json line 4072
Quoted 1 line
qapi/block-core.json +1
-0+# @subformat The subformat of the VMDK image. Default: "monolithicsparse".
monolithicSparse
qapi/block-core.json line 4076
Quoted 4 lines
qapi/block-core.json +4
-0+# @backing-file The path of backing file. Default: no backing file is used. +# @adapter-type The adapter type used to fill in the descriptor. Default: ide. +# @hwversion Hardware version. The meaningful options are "4" or "6". +# Defaulted to "4".
Default: "4"
Quoted 4 lines
qapi/block-core.json +4
-0+# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats. +# Default: false. +# +# Since: 4.0
qapi/block-core.json line 4090
Quoted 10 lines
qapi/block-core.json +10
-0+## +{ 'struct': 'BlockdevCreateOptionsVmdk', + 'data': { 'file': 'BlockdevRef', + 'size': 'size', + '*extents': ['BlockdevRef'], + '*subformat': 'BlockdevVmdkSubformat', + '*backing-file': 'str', + '*adapter-type': 'BlockdevVmdkAdapterType', + '*hwversion': 'str', + '*zeroed-grain': 'bool' } }
@zeroed-grain is undocumented.
Quoted 44 lines
qapi/block-core.json +3
-0+ + ## # @SheepdogRedundancyType: #@@ -4215,6 +4284,7 @@ 'ssh': 'BlockdevCreateOptionsSsh', 'vdi': 'BlockdevCreateOptionsVdi', 'vhdx': 'BlockdevCreateOptionsVhdx', + 'vmdk': 'BlockdevCreateOptionsVmdk', 'vpc': 'BlockdevCreateOptionsVpc' } }qapi/qapi-schema.json +1
-0@@ -66,6 +66,7 @@ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status 'CpuInfoMIPS', # PC, visible through query-cpu 'CpuInfoTricore', # PC, visible through query-cpu + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings 'QapiErrorClass', # all members, visible through errors 'UuidInfo', # UUID, visible through query-uuid 'X86CPURegister32', # all members, visible indirectly through qom-getblock/vmdk.c +9
-2@@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,return VMDK_OK ; } -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts, -Error **errp) +/* +* idx == 0: get or create the descriptor file (also the image file if in a +* non-split format. +* idx >= 1: get the n-th extent if in a split subformat +*/ +typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size, + int idx, + bool flat, + bool split,
block/vmdk.c static int filename_decompose(const char *filename, char *path, char *prefix, · line 1953
Quoted 10 lines
block/vmdk.c +10
-0+bool compress, + bool zeroed_grain, + void *opaque, + Error **errp); + +static void vmdk_desc_add_extent (GString *desc, +const char *extent_line_fmt, +int64_t size,const char *filename) +{ +char *basename =g_path_get_basename (filename);
I like a blank line between declarations and statements.
Quoted 87 lines
block/vmdk.c +26
-35+g_string_append_printf (desc, extent_line_fmt, +DIV_ROUND_UP (size,BDRV_SECTOR_SIZE ), basename); + +g_free (basename); +} + +static int coroutine_fn vmdk_co_do_create(int64_t size, +BlockdevVmdkSubformat subformat, +BlockdevVmdkAdapterType adapter_type, +const char *backing_file, +const char *hw_version, +bool compat6, +bool zeroed_grain, +vmdk_create_extent_fn extent_fn, +void *opaque, +Error **errp) { -int idx =0 ; -BlockBackend *new_blk =NULL ; +int extent_idx; +BlockBackend *blk =NULL ;Error *local_err =NULL ;char *desc =NULL ; -int64_t total_size =0 , filesize; -char *adapter_type =NULL ; -char *backing_file =NULL ; -char *hw_version =NULL ; -char *fmt =NULL ;int ret =0 ;bool flat, split, compress;GString *ext_desc_lines; -char *path =g_malloc0 (PATH_MAX ); -char *prefix =g_malloc0 (PATH_MAX ); -char *postfix =g_malloc0 (PATH_MAX ); -char *desc_line =g_malloc0 (BUF_SIZE ); -char *ext_filename =g_malloc0 (PATH_MAX ); -char *desc_filename =g_malloc0 (PATH_MAX );const int64_t split_size =0x80000000 ;/* VMDK has constant split size */ -const char *desc_extent_line; +int64_t extent_size; +int64_t created_size =0 ; +const char *extent_line_fmt;char *parent_desc_line =g_malloc0 (BUF_SIZE );uint32_t parent_cid =0xffffffff ;uint32_t number_heads =16 ; -bool zeroed_grain = false;uint32_t desc_offset =0 , desc_len;const char desc_template[] ="# Disk DescriptorFile\n" @@ -1982,71 +2005,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts ext_desc_lines =g_string_new (NULL ); -if (filename_decompose (filename, path, prefix, postfix,PATH_MAX , errp)) { - ret = -EINVAL ; - goto exit; - }/* Read out options */ - total_size =ROUND_UP (qemu_opt_get_size_del (opts,BLOCK_OPT_SIZE ,0 ), -BDRV_SECTOR_SIZE ); - adapter_type =qemu_opt_get_del (opts,BLOCK_OPT_ADAPTER_TYPE ); - backing_file =qemu_opt_get_del (opts,BLOCK_OPT_BACKING_FILE ); - hw_version =qemu_opt_get_del (opts,BLOCK_OPT_HWVERSION ); -if (qemu_opt_get_bool_del (opts,BLOCK_OPT_COMPAT6 , false)) { -if (strcmp (hw_version,"undefined" )) { +if (compat6) { +if (hw_version) {error_setg (errp,"compat6 cannot be enabled with hwversion set" ); ret = -EINVAL ; goto exit; } -g_free (hw_version); - hw_version =g_strdup ("6" ); - } -if (strcmp (hw_version,"undefined" ) ==0 ) { -g_free (hw_version); - hw_version =g_strdup ("4" ); + hw_version ="6" ; } - fmt =qemu_opt_get_del (opts,BLOCK_OPT_SUBFMT ); -if (qemu_opt_get_bool_del (opts,BLOCK_OPT_ZEROED_GRAIN , false)) { - zeroed_grain = true; +if (!hw_version) { + hw_version ="4" ; }
block/vmdk.c static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts · line 2023
Quoted 10 lines
block/vmdk.c +0
-10-if (!adapter_type) { - adapter_type =g_strdup ("ide" ); - }else if (strcmp (adapter_type,"ide" ) && -strcmp (adapter_type,"buslogic" ) && -strcmp (adapter_type,"lsilogic" ) && -strcmp (adapter_type,"legacyESX" )) { -error_setg (errp,"Unknown adapter type: '%s'" , adapter_type); - ret = -EINVAL ; - goto exit; - }
Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug or feature?
Quoted 38 lines
block/vmdk.c +9
-20-if (strcmp (adapter_type,"ide" ) !=0 ) { +if (adapter_type !=BLOCKDEV_VMDK_ADAPTER_TYPE_IDE ) {/* that's the number of heads with which vmware operates when creating, exporting, etc. vmdk files with a non-ide adapter type */ number_heads =255 ; } -if (!fmt) { -/* Default format to monolithicSparse */ - fmt =g_strdup ("monolithicSparse" ); - }else if (strcmp (fmt,"monolithicFlat" ) && -strcmp (fmt,"monolithicSparse" ) && -strcmp (fmt,"twoGbMaxExtentSparse" ) && -strcmp (fmt,"twoGbMaxExtentFlat" ) && -strcmp (fmt,"streamOptimized" )) { -error_setg (errp,"Unknown subformat: '%s'" , fmt); - ret = -EINVAL ; - goto exit; - } - split = !(strcmp (fmt,"twoGbMaxExtentFlat" ) && -strcmp (fmt,"twoGbMaxExtentSparse" )); - flat = !(strcmp (fmt,"monolithicFlat" ) && -strcmp (fmt,"twoGbMaxExtentFlat" )); - compress = !strcmp (fmt,"streamOptimized" ); + split = (subformat ==BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT ) || + (subformat ==BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTSPARSE ); + flat = (subformat ==BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICFLAT ) || + (subformat ==BLOCKDEV_VMDK_SUBFORMAT_TWOGBMAXEXTENTFLAT ); + compress = subformat ==BLOCKDEV_VMDK_SUBFORMAT_STREAMOPTIMIZED ; +if (flat) { - desc_extent_line ="RW %" PRId64" FLAT \"%s\" 0\n" ; + extent_line_fmt ="RW %" PRId64" FLAT \"%s\" 0\n" ; }else { - desc_extent_line ="RW %" PRId64" SPARSE \"%s\"\n" ; + extent_line_fmt ="RW %" PRId64" SPARSE \"%s\"\n" ; }if (flat && backing_file) {error_setg (errp,"Flat image can't have backing file" );
[...]
Re: [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
Am 07.12.2018 um 08:10 hat Markus Armbruster geschrieben:
Quoted 59 lines
This is a reasonably careful review of the QAPI-related parts, but more of an eye-over for the remainder.
Quoted 40 lines · Kevin Wolf
From: Fam Zheng <[email protected]>
This makes VMDK support blockdev-create. The implementation reuses the image creation code in
vmdk_co_create_optswhich now acceptes a callback pointer to "retrieve" BlockBackend pointers from the caller. This way we separate the logic between file/extent acquisition and initialization.The QAPI command parameters are mostly the same as the old
create_optsexcept the dropped legacy @compat6 switch, which is redundant with @hwversion.Signed-off-by Fam Zheng <[email protected]>Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 70 +++++++ qapi/qapi-schema.json | 1 + block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ 3 files changed, 396 insertions(+), 127 deletions(-)qapi/block-core.json +13
-0@@ -4021,6 +4021,75 @@ 'size': 'size', '*cluster-size' : 'size' } } +## +# @BlockdevVmdkSubformat: +# +# Subformat options for VMDK images +# +# @monolithicSparse: Single file image with sparse cluster allocation +# +# @monolithicFlat: Single flat data image and a descriptor file +# +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent +# files, in addition to a descriptor file +# +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extentqapi/block-core.json line 4046
Quoted 10 lines
qapi/block-core.json +10
-0+# files, in addition to a descriptor file +# +# @streamOptimized: Single file image sparse cluster allocation, optimized +# for streaming over network. +# +# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkSubformat', + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse', + 'twoGbMaxExtentFlat', 'streamOptimized'] }Don't conform to the QAPI rules on names "to match VMDK spec spellings" (see qapi-schema.json below). We discussed this in review of v1.
Quoted 8 lines
PRO: matches the VMDK spec (whatever that's worth), keeps the code simple.
CON: the non-conforming names become part of the stable QMP interface, in the argument of command blockdev-create.
I don't like the CON, but I'm willing to tolerate it in the name of simplicity.
I don't really have a opinion here. It seems this is what you had agreed on in v1 and it still feels acceptable (even if not perfect) to you, so I'll stick with it.
Quoted 6 lines
Quoted 6 lines
qapi/block-core.json +6
-0+ +## +# @BlockdevVmdkAdapterType: +# +# Adapter type info for VMDK images +#
Quoted 8 lines
qapi/block-core.json line 4056
Quoted 4 lines
qapi/block-core.json +4
-0+# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkAdapterType', + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }May I have hyphens in the composite nouns? Hmm, might be the way they are to match VMDK spec spellings or for backward compatibility. I guess we'll see below.
These are literal values to be written into the VMDK descriptor files, so they have to use this spelling. If we want to have a different spelling in QMP, then we'll have to translate internally.
I guess this is the same argument as for BlockdevVmdkSubformat above. For consistency, we should use the spec spellings in both cases or not at all.
Quoted 16 lines
qapi/block-core.json line 4069
Quoted 13 lines
qapi/block-core.json +13
-0+ +## +# @BlockdevCreateOptionsVmdk: +# +# Driver specific image creation options for VMDK. +# +# @file Where to store the new image file. This refers to the image +# file for monolithcSparse and streamOptimized format, or the +# descriptor file for other formats. +# @size Size of the virtual disk in bytes +# @extents Where to store the data extents. Required for monolithcflat, +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For +# monolithicflat, only one entry is required; formonolithicFlat
Quoted 5 lines
qapi/block-core.json line 4071
Quoted 2 lines
qapi/block-core.json +2
-0+# twoGbMaxExtent* formats, the number of entries required is +# calculated as extent_number = virtual_size / 2GB.Is it okay to supply more entries than required, or do I have to supply exactly the right number?
If I understand the code correctly, additional extents are silently ignored.
Quoted 10 lines
qapi/block-core.json line 4072
Quoted 1 line
qapi/block-core.json +1
-0+# @subformat The subformat of the VMDK image. Default: "monolithicsparse".monolithicSparse
qapi/block-core.json line 4076
Quoted 4 lines
qapi/block-core.json +4
-0+# @backing-file The path of backing file. Default: no backing file is used. +# @adapter-type The adapter type used to fill in the descriptor. Default: ide. +# @hwversion Hardware version. The meaningful options are "4" or "6". +# Defaulted to "4".Default: "4"
Fixed all the spelling changes.
Quoted 8 lines
Quoted 8 lines
qapi/block-core.json +8
-0+# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats. +# Default: false. +# +# Since: 4.0 +## +{ 'struct': 'BlockdevCreateOptionsVmdk', + 'data': { 'file': 'BlockdevRef', + 'size': 'size',
Quoted 8 lines
qapi/block-core.json line 4090
Quoted 6 lines
qapi/block-core.json +6
-0+ '*extents': ['BlockdevRef'], + '*subformat': 'BlockdevVmdkSubformat', + '*backing-file': 'str', + '*adapter-type': 'BlockdevVmdkAdapterType', + '*hwversion': 'str', + '*zeroed-grain': 'bool' } }@zeroed-grain is undocumented.
It's right there at the beginning of the quote after your last comment?
Quoted 48 lines
Quoted 48 lines
qapi/block-core.json +3
-0+ + ## # @SheepdogRedundancyType: #@@ -4215,6 +4284,7 @@ 'ssh': 'BlockdevCreateOptionsSsh', 'vdi': 'BlockdevCreateOptionsVdi', 'vhdx': 'BlockdevCreateOptionsVhdx', + 'vmdk': 'BlockdevCreateOptionsVmdk', 'vpc': 'BlockdevCreateOptionsVpc' } }qapi/qapi-schema.json +1
-0@@ -66,6 +66,7 @@ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status 'CpuInfoMIPS', # PC, visible through query-cpu 'CpuInfoTricore', # PC, visible through query-cpu + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings 'QapiErrorClass', # all members, visible through errors 'UuidInfo', # UUID, visible through query-uuid 'X86CPURegister32', # all members, visible indirectly through qom-getblock/vmdk.c +13
-2@@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,return VMDK_OK ; } -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts, -Error **errp) +/* +* idx == 0: get or create the descriptor file (also the image file if in a +* non-split format. +* idx >= 1: get the n-th extent if in a split subformat +*/ +typedef BlockBackend *(*vmdk_create_extent_fn )(int64_t size, +int idx, +bool flat, +bool split, +bool compress, +bool zeroed_grain, +void *opaque, +Error **errp);
Quoted 8 lines
block/vmdk.c static int filename_decompose(const char *filename, char *path, char *prefix, · line 1953
Quoted 6 lines
block/vmdk.c +6
-0+ +static void vmdk_desc_add_extent (GString *desc, +const char *extent_line_fmt, +int64_t size,const char *filename) +{ +char *basename =g_path_get_basename (filename);I like a blank line between declarations and statements.
Matter of taste. But okay, I already changed this function, so I'll give you this one.
Quoted 87 lines
Quoted 87 lines
block/vmdk.c +26
-35+g_string_append_printf (desc, extent_line_fmt, +DIV_ROUND_UP (size,BDRV_SECTOR_SIZE ), basename); + +g_free (basename); +} + +static int coroutine_fn vmdk_co_do_create(int64_t size, +BlockdevVmdkSubformat subformat, +BlockdevVmdkAdapterType adapter_type, +const char *backing_file, +const char *hw_version, +bool compat6, +bool zeroed_grain, +vmdk_create_extent_fn extent_fn, +void *opaque, +Error **errp) { -int idx =0 ; -BlockBackend *new_blk =NULL ; +int extent_idx; +BlockBackend *blk =NULL ;Error *local_err =NULL ;char *desc =NULL ; -int64_t total_size =0 , filesize; -char *adapter_type =NULL ; -char *backing_file =NULL ; -char *hw_version =NULL ; -char *fmt =NULL ;int ret =0 ;bool flat, split, compress;GString *ext_desc_lines; -char *path =g_malloc0 (PATH_MAX ); -char *prefix =g_malloc0 (PATH_MAX ); -char *postfix =g_malloc0 (PATH_MAX ); -char *desc_line =g_malloc0 (BUF_SIZE ); -char *ext_filename =g_malloc0 (PATH_MAX ); -char *desc_filename =g_malloc0 (PATH_MAX );const int64_t split_size =0x80000000 ;/* VMDK has constant split size */ -const char *desc_extent_line; +int64_t extent_size; +int64_t created_size =0 ; +const char *extent_line_fmt;char *parent_desc_line =g_malloc0 (BUF_SIZE );uint32_t parent_cid =0xffffffff ;uint32_t number_heads =16 ; -bool zeroed_grain = false;uint32_t desc_offset =0 , desc_len;const char desc_template[] ="# Disk DescriptorFile\n" @@ -1982,71 +2005,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts ext_desc_lines =g_string_new (NULL ); -if (filename_decompose (filename, path, prefix, postfix,PATH_MAX , errp)) { - ret = -EINVAL ; - goto exit; - }/* Read out options */ - total_size =ROUND_UP (qemu_opt_get_size_del (opts,BLOCK_OPT_SIZE ,0 ), -BDRV_SECTOR_SIZE ); - adapter_type =qemu_opt_get_del (opts,BLOCK_OPT_ADAPTER_TYPE ); - backing_file =qemu_opt_get_del (opts,BLOCK_OPT_BACKING_FILE ); - hw_version =qemu_opt_get_del (opts,BLOCK_OPT_HWVERSION ); -if (qemu_opt_get_bool_del (opts,BLOCK_OPT_COMPAT6 , false)) { -if (strcmp (hw_version,"undefined" )) { +if (compat6) { +if (hw_version) {error_setg (errp,"compat6 cannot be enabled with hwversion set" ); ret = -EINVAL ; goto exit; } -g_free (hw_version); - hw_version =g_strdup ("6" ); - } -if (strcmp (hw_version,"undefined" ) ==0 ) { -g_free (hw_version); - hw_version =g_strdup ("4" ); + hw_version ="6" ; } - fmt =qemu_opt_get_del (opts,BLOCK_OPT_SUBFMT ); -if (qemu_opt_get_bool_del (opts,BLOCK_OPT_ZEROED_GRAIN , false)) { - zeroed_grain = true; +if (!hw_version) { + hw_version ="4" ; }
Quoted 13 lines
block/vmdk.c static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts · line 2023
Quoted 10 lines
block/vmdk.c +0
-10-if (!adapter_type) { - adapter_type =g_strdup ("ide" ); - }else if (strcmp (adapter_type,"ide" ) && -strcmp (adapter_type,"buslogic" ) && -strcmp (adapter_type,"lsilogic" ) && -strcmp (adapter_type,"legacyESX" )) { -error_setg (errp,"Unknown adapter type: '%s'" , adapter_type); - ret = -EINVAL ; - goto exit; - }Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug or feature?
Good catch!
Who knows? But it's not advertised as a feature in the commit message, and I can only see the spelling "legacyESX" in the spec, so I'll change the QAPI definition to "legacyESX". Leaving things as they were should be safe.
Fam also made the value case insensitive when parsing QemuOpts in vmdk_co_create_opts() to compensate for the change. I think this can also go away then?
Kevin
Re: [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback
Quoted 66 lines · Kevin Wolf
Am 07.12.2018 um 08:10 hat Markus Armbruster geschrieben:
Quoted 65 lines
This is a reasonably careful review of the QAPI-related parts, but more of an eye-over for the remainder.
Quoted 40 lines · Kevin Wolf
From: Fam Zheng <[email protected]>
This makes VMDK support blockdev-create. The implementation reuses the image creation code in
vmdk_co_create_optswhich now acceptes a callback pointer to "retrieve" BlockBackend pointers from the caller. This way we separate the logic between file/extent acquisition and initialization.The QAPI command parameters are mostly the same as the old
create_optsexcept the dropped legacy @compat6 switch, which is redundant with @hwversion.Signed-off-by Fam Zheng <[email protected]>Signed-off-by Kevin Wolf <[email protected]>--- qapi/block-core.json | 70 +++++++ qapi/qapi-schema.json | 1 + block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ 3 files changed, 396 insertions(+), 127 deletions(-)qapi/block-core.json +13
-0@@ -4021,6 +4021,75 @@ 'size': 'size', '*cluster-size' : 'size' } } +## +# @BlockdevVmdkSubformat: +# +# Subformat options for VMDK images +# +# @monolithicSparse: Single file image with sparse cluster allocation +# +# @monolithicFlat: Single flat data image and a descriptor file +# +# @twoGbMaxExtentSparse: Data is split into 2GB (per virtual LBA) sparse extent +# files, in addition to a descriptor file +# +# @twoGbMaxExtentFlat: Data is split into 2GB (per virtual LBA) flat extentqapi/block-core.json line 4046
Quoted 10 lines
qapi/block-core.json +10
-0+# files, in addition to a descriptor file +# +# @streamOptimized: Single file image sparse cluster allocation, optimized +# for streaming over network. +# +# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkSubformat', + 'data': [ 'monolithicSparse', 'monolithicFlat', 'twoGbMaxExtentSparse', + 'twoGbMaxExtentFlat', 'streamOptimized'] }Don't conform to the QAPI rules on names "to match VMDK spec spellings" (see qapi-schema.json below). We discussed this in review of v1.
PRO: matches the VMDK spec (whatever that's worth), keeps the code simple.
CON: the non-conforming names become part of the stable QMP interface, in the argument of command blockdev-create.
Quoted 6 lines
Quoted 2 lines
I don't like the CON, but I'm willing to tolerate it in the name of simplicity.
I don't really have a opinion here. It seems this is what you had agreed on in v1 and it still feels acceptable (even if not perfect) to you, so I'll stick with it.
That's okay. I think I'd like the alternatives less.
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
qapi/block-core.json +6
-0+ +## +# @BlockdevVmdkAdapterType: +# +# Adapter type info for VMDK images +#Quoted 8 lines
qapi/block-core.json line 4056
Quoted 4 lines
qapi/block-core.json +4
-0+# Since: 4.0 +## +{ 'enum': 'BlockdevVmdkAdapterType', + 'data': [ 'ide', 'buslogic', 'lsilogic', 'legacyesx'] }May I have hyphens in the composite nouns? Hmm, might be the way they are to match VMDK spec spellings or for backward compatibility. I guess we'll see below.
Quoted 7 lines
These are literal values to be written into the VMDK descriptor files, so they have to use this spelling. If we want to have a different spelling in QMP, then we'll have to translate internally.
I guess this is the same argument as for BlockdevVmdkSubformat above. For consistency, we should use the spec spellings in both cases or not at all.
Makes sense.
Quoted 16 lines
Quoted 16 lines
qapi/block-core.json line 4069
Quoted 13 lines
qapi/block-core.json +13
-0+ +## +# @BlockdevCreateOptionsVmdk: +# +# Driver specific image creation options for VMDK. +# +# @file Where to store the new image file. This refers to the image +# file for monolithcSparse and streamOptimized format, or the +# descriptor file for other formats. +# @size Size of the virtual disk in bytes +# @extents Where to store the data extents. Required for monolithcflat, +# twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For +# monolithicflat, only one entry is required; formonolithicFlat
Quoted 8 lines
Quoted 5 lines
qapi/block-core.json line 4071
Quoted 2 lines
qapi/block-core.json +2
-0+# twoGbMaxExtent* formats, the number of entries required is +# calculated as extent_number = virtual_size / 2GB.Is it okay to supply more entries than required, or do I have to supply exactly the right number?
If I understand the code correctly, additional extents are silently ignored.
I see you're fixing that in v4. Good.
Quoted 30 lines
Quoted 10 lines
qapi/block-core.json line 4072
Quoted 1 line
qapi/block-core.json +1
-0+# @subformat The subformat of the VMDK image. Default: "monolithicsparse".monolithicSparse
qapi/block-core.json line 4076
Quoted 4 lines
qapi/block-core.json +4
-0+# @backing-file The path of backing file. Default: no backing file is used. +# @adapter-type The adapter type used to fill in the descriptor. Default: ide. +# @hwversion Hardware version. The meaningful options are "4" or "6". +# Defaulted to "4".Default: "4"
Fixed all the spelling changes.
Quoted 8 lines
Quoted 8 lines
qapi/block-core.json +8
-0+# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats. +# Default: false. +# +# Since: 4.0 +## +{ 'struct': 'BlockdevCreateOptionsVmdk', + 'data': { 'file': 'BlockdevRef', + 'size': 'size',Quoted 8 lines
qapi/block-core.json line 4090
Quoted 6 lines
qapi/block-core.json +6
-0+ '*extents': ['BlockdevRef'], + '*subformat': 'BlockdevVmdkSubformat', + '*backing-file': 'str', + '*adapter-type': 'BlockdevVmdkAdapterType', + '*hwversion': 'str', + '*zeroed-grain': 'bool' } }@zeroed-grain is undocumented.
Quoted 1 line
It's right there at the beginning of the quote after your last comment?
You're right, sorry for the noise.
Quoted 161 lines
Quoted 48 lines
Quoted 48 lines
qapi/block-core.json +3
-0+ + ## # @SheepdogRedundancyType: #@@ -4215,6 +4284,7 @@ 'ssh': 'BlockdevCreateOptionsSsh', 'vdi': 'BlockdevCreateOptionsVdi', 'vhdx': 'BlockdevCreateOptionsVhdx', + 'vmdk': 'BlockdevCreateOptionsVmdk', 'vpc': 'BlockdevCreateOptionsVpc' } }qapi/qapi-schema.json +1
-0@@ -66,6 +66,7 @@ 'ACPISlotType', # DIMM, visible through query-acpi-ospm-status 'CpuInfoMIPS', # PC, visible through query-cpu 'CpuInfoTricore', # PC, visible through query-cpu + 'BlockdevVmdkSubformat',# all members, to match VMDK spec spellings 'QapiErrorClass', # all members, visible through errors 'UuidInfo', # UUID, visible through query-uuid 'X86CPURegister32', # all members, visible indirectly through qom-getblock/vmdk.c +13
-2@@ -1932,33 +1932,56 @@ static int filename_decompose(const char *filename, char *path, char *prefix,return VMDK_OK ; } -static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts, -Error **errp) +/* +* idx == 0: get or create the descriptor file (also the image file if in a +* non-split format. +* idx >= 1: get the n-th extent if in a split subformat +*/ +typedef BlockBackend *(*vmdk_create_extent_fn )(int64_t size, +int idx, +bool flat, +bool split, +bool compress, +bool zeroed_grain, +void *opaque, +Error **errp);Quoted 8 lines
block/vmdk.c static int filename_decompose(const char *filename, char *path, char *prefix, · line 1953
Quoted 6 lines
block/vmdk.c +6
-0+ +static void vmdk_desc_add_extent (GString *desc, +const char *extent_line_fmt, +int64_t size,const char *filename) +{ +char *basename =g_path_get_basename (filename);I like a blank line between declarations and statements.
Matter of taste. But okay, I already changed this function, so I'll give you this one.
Quoted 87 lines
Quoted 87 lines
block/vmdk.c +26
-35+g_string_append_printf (desc, extent_line_fmt, +DIV_ROUND_UP (size,BDRV_SECTOR_SIZE ), basename); + +g_free (basename); +} + +static int coroutine_fn vmdk_co_do_create(int64_t size, +BlockdevVmdkSubformat subformat, +BlockdevVmdkAdapterType adapter_type, +const char *backing_file, +const char *hw_version, +bool compat6, +bool zeroed_grain, +vmdk_create_extent_fn extent_fn, +void *opaque, +Error **errp) { -int idx =0 ; -BlockBackend *new_blk =NULL ; +int extent_idx; +BlockBackend *blk =NULL ;Error *local_err =NULL ;char *desc =NULL ; -int64_t total_size =0 , filesize; -char *adapter_type =NULL ; -char *backing_file =NULL ; -char *hw_version =NULL ; -char *fmt =NULL ;int ret =0 ;bool flat, split, compress;GString *ext_desc_lines; -char *path =g_malloc0 (PATH_MAX ); -char *prefix =g_malloc0 (PATH_MAX ); -char *postfix =g_malloc0 (PATH_MAX ); -char *desc_line =g_malloc0 (BUF_SIZE ); -char *ext_filename =g_malloc0 (PATH_MAX ); -char *desc_filename =g_malloc0 (PATH_MAX );const int64_t split_size =0x80000000 ;/* VMDK has constant split size */ -const char *desc_extent_line; +int64_t extent_size; +int64_t created_size =0 ; +const char *extent_line_fmt;char *parent_desc_line =g_malloc0 (BUF_SIZE );uint32_t parent_cid =0xffffffff ;uint32_t number_heads =16 ; -bool zeroed_grain = false;uint32_t desc_offset =0 , desc_len;const char desc_template[] ="# Disk DescriptorFile\n" @@ -1982,71 +2005,35 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts ext_desc_lines =g_string_new (NULL ); -if (filename_decompose (filename, path, prefix, postfix,PATH_MAX , errp)) { - ret = -EINVAL ; - goto exit; - }/* Read out options */ - total_size =ROUND_UP (qemu_opt_get_size_del (opts,BLOCK_OPT_SIZE ,0 ), -BDRV_SECTOR_SIZE ); - adapter_type =qemu_opt_get_del (opts,BLOCK_OPT_ADAPTER_TYPE ); - backing_file =qemu_opt_get_del (opts,BLOCK_OPT_BACKING_FILE ); - hw_version =qemu_opt_get_del (opts,BLOCK_OPT_HWVERSION ); -if (qemu_opt_get_bool_del (opts,BLOCK_OPT_COMPAT6 , false)) { -if (strcmp (hw_version,"undefined" )) { +if (compat6) { +if (hw_version) {error_setg (errp,"compat6 cannot be enabled with hwversion set" ); ret = -EINVAL ; goto exit; } -g_free (hw_version); - hw_version =g_strdup ("6" ); - } -if (strcmp (hw_version,"undefined" ) ==0 ) { -g_free (hw_version); - hw_version =g_strdup ("4" ); + hw_version ="6" ; } - fmt =qemu_opt_get_del (opts,BLOCK_OPT_SUBFMT ); -if (qemu_opt_get_bool_del (opts,BLOCK_OPT_ZEROED_GRAIN , false)) { - zeroed_grain = true; +if (!hw_version) { + hw_version ="4" ; }Quoted 13 lines
block/vmdk.c static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts · line 2023
Quoted 10 lines
block/vmdk.c +0
-10-if (!adapter_type) { - adapter_type =g_strdup ("ide" ); - }else if (strcmp (adapter_type,"ide" ) && -strcmp (adapter_type,"buslogic" ) && -strcmp (adapter_type,"lsilogic" ) && -strcmp (adapter_type,"legacyESX" )) { -error_setg (errp,"Unknown adapter type: '%s'" , adapter_type); - ret = -EINVAL ; - goto exit; - }Old code recognizes "legacyESX". New code recognizes "legacyesx". Bug or feature?
Quoted 6 lines
Good catch!
Who knows? But it's not advertised as a feature in the commit message, and I can only see the spelling "legacyESX" in the spec, so I'll change the QAPI definition to "legacyESX". Leaving things as they were should be safe.
I suspect it's a remnant of v1, where Fam used all lower case enum values, plus code to match them case insensitively.
Quoted 3 lines
Fam also made the value case insensitive when parsing QemuOpts in
vmdk_co_create_opts()to compensate for the change. I think this can also go away then?
Yes, please.
PATCH v3 3/4 iotests: Filter cid numbers in VMDK extent info
In reply to Kevin WolfFrom: Fam Zheng <[email protected]>
--- tests/qemu-iotests/common.filter | 1 + tests/qemu-iotests/iotests.py | 1 + 2 files changed, 2 insertions(+)
tests/qemu-iotests/common.filter +1 -0
@@ -165,6 +165,7 @@ _filter_img_info() -e "/table_size: [0-9]\\+/d" \ -e "/compat: '[^']*'/d" \ -e "/compat6: \\(on\\|off\\)/d" \ + -e "s/cid: [0-9]\+/cid: XXXXXXXXXX/" \ -e "/static: \\(on\\|off\\)/d" \ -e "/zeroed_grain: \\(on\\|off\\)/d" \ -e "/subformat: '[^']*'/d" \
tests/qemu-iotests/iotests.py +1 -0
@@ -248,6 +248,7 @@ def filter_img_info(output, filename): .replace(imgfmt, 'IMGFMT') line = re.sub('iters: [0-9]+', 'iters: XXX', line) line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line) + line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line) lines.append(line) return '\n'.join(lines)
Signature · 1 line
2.19.2
PATCH v3 4/4 iotests: Add VMDK tests for blockdev-create
In reply to Kevin Wolf--- tests/qemu-iotests/237 | 201 ++++++++++++++++++++++++ tests/qemu-iotests/237.out | 309 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 511 insertions(+) create mode 100755 tests/qemu-iotests/237 create mode 100644 tests/qemu-iotests/237.out
tests/qemu-iotests/237 new file +201 -0
@@ -0,0 +1,201 @@ +#!/usr/bin/env python +# +# Test vmdk and file image creation +# +# Copyright (C) 2018 Red Hat, Inc. +# +# Creator/Owner: Kevin Wolf <[email protected]> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +import iotests +from iotests import imgfmt + +iotests.verify_image_format(supported_fmts=['vmdk']) + +def blockdev_create(vm, options): + result = vm.qmp_log('blockdev-create', job_id='job0', options=options) + + if 'return' in result: + assert result['return'] == {} + vm.run_job('job0') + iotests.log("") + +with iotests.FilePath('t.vmdk') as disk_path, \ + iotests.FilePath('t.vmdk.1') as extent1_path, \ + iotests.FilePath('t.vmdk.2') as extent2_path, \ + iotests.FilePath('t.vmdk.3') as extent3_path, \ + iotests.VM() as vm: + + # + # Successful image creation (defaults) + # + iotests.log("=== Successful image creation (defaults) ===") + iotests.log("") + + size = 5 * 1024 * 1024 * 1024 + + vm.launch() + blockdev_create(vm, { 'driver': 'file', + 'filename': disk_path, + 'size': 0 }) + + vm.qmp_log('blockdev-add', driver='file', filename=disk_path, + node_name='imgfile') + + blockdev_create(vm, { 'driver': imgfmt, + 'file': 'imgfile', + 'size': size }) + vm.shutdown() + + iotests.img_info_log(disk_path) + + # + # Successful image creation (inline blockdev-add, explicit defaults) + # + iotests.log("=== Successful image creation (inline blockdev-add, explicit defaults) ===") + iotests.log("") + + # Choose a different size to show that we got a new image + size = 64 * 1024 * 1024 + + vm.launch() + blockdev_create(vm, { 'driver': 'file', + 'filename': disk_path, + 'size': 0 }) + + blockdev_create(vm, { 'driver': imgfmt, + 'file': { + 'driver': 'file', + 'filename': disk_path, + }, + 'size': size, + 'extents': [], + 'subformat': 'monolithicSparse', + 'adapter-type': 'ide', + 'hwversion': '4', + 'zeroed-grain': False }) + vm.shutdown() + + iotests.img_info_log(disk_path) + + # + # Successful image creation (non-default options) + # + iotests.log("=== Successful image creation (with non-default options) ===") + iotests.log("") + + # Choose a different size to show that we got a new image + size = 32 * 1024 * 1024 + + vm.launch() + blockdev_create(vm, { 'driver': 'file', + 'filename': disk_path, + 'size': 0 }) + + blockdev_create(vm, { 'driver': imgfmt, + 'file': { + 'driver': 'file', + 'filename': disk_path, + }, + 'size': size, + 'extents': [], + 'subformat': 'monolithicSparse', + 'adapter-type': 'buslogic', + 'zeroed-grain': True }) + vm.shutdown() + + iotests.img_info_log(disk_path) + + # + # Invalid BlockdevRef + # + iotests.log("=== Invalid BlockdevRef ===") + iotests.log("") + + vm.launch() + blockdev_create(vm, { 'driver': imgfmt, + 'file': "this doesn't exist", + 'size': size }) + vm.shutdown() + + # + # Other subformats + # + iotests.log("=== Other subformats ===") + iotests.log("") + + for path in [ extent1_path, extent2_path, extent3_path ]: + msg = iotests.qemu_img_pipe('create', '-f', imgfmt, path, '0') + iotests.log(msg, [iotests.filter_testfiles]) + + vm.add_blockdev('driver=file,filename=%s,node-name=node0' % (disk_path)) + vm.add_blockdev('driver=file,filename=%s,node-name=ext1' % (extent1_path)) + vm.add_blockdev('driver=file,filename=%s,node-name=ext2' % (extent2_path)) + vm.add_blockdev('driver=file,filename=%s,node-name=ext3' % (extent3_path)) + + # Missing extent + iotests.log("== Missing extent ==") + iotests.log("") + + vm.launch() + blockdev_create(vm, { 'driver': imgfmt, + 'file': 'node0', + 'size': size, + 'subformat': 'monolithicFlat' }) + vm.shutdown() + + # Correct extent + iotests.log("== Correct extent ==") + iotests.log("") + + vm.launch() + blockdev_create(vm, { 'driver': imgfmt, + 'file': 'node0', + 'size': size, + 'subformat': 'monolithicFlat', + 'extents': ['ext1'] }) + vm.shutdown() + + # Extra extent + iotests.log("== Extra extent ==") + iotests.log("") + + vm.launch() + blockdev_create(vm, { 'driver': imgfmt, + 'file': 'node0', + 'size': 512, + 'subformat': 'monolithicFlat', + 'extents': ['ext1', 'ext2', 'ext3'] }) + vm.shutdown() + + # Split formats + iotests.log("== Split formats ==") + iotests.log("") + + for size in [ 512, 1073741824, 2147483648, 5368709120 ]: + for subfmt in [ 'twoGbMaxExtentFlat', 'twoGbMaxExtentSparse' ]: + iotests.log("= %s %d =" % (subfmt, size)) + iotests.log("") + + vm.launch() + blockdev_create(vm, { 'driver': imgfmt, + 'file': 'node0', + 'size': size, + 'subformat': subfmt, + 'extents': ['ext1', 'ext2', 'ext3'] }) + vm.shutdown() + + iotests.img_info_log(disk_path)
tests/qemu-iotests/237.out new file +309 -0
309 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -233,3 +233,4 @@ 233 auto quick 234 auto quick migration 235 auto quick +237 rw auto quick
Signature · 1 line
2.19.2
Patchew URL: https://patchew.org/QEMU/[email protected]/
Patchew URL: https://patchew.org/QEMU/[email protected]/
Hi,
This series seems to have some coding style problems. See output below for more information:
Type: series
Subject: [Qemu-devel] [PATCH v3 0/4] vmdk: Implement blockdev-create
Message-id: [email protected]
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test'
72ea84d iotests: Add VMDK tests for blockdev-create e2fa3cf iotests: Filter cid numbers in VMDK extent info fb76434 vmdk: Implement .bdrv_co_create callback d16851a vmdk: Refactor vmdk_create_extent
=== OUTPUT BEGIN ===
Checking PATCH 1/4: vmdk: Refactor vmdk_create_extent...
WARNING: line over 80 characters
#124: FILE: block/vmdk.c:2114:
+ flat, compress, zeroed_grain, NULL, opts, errp)) {
total: 0 errors, 1 warnings, 104 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
Checking PATCH 2/4: vmdk: Implement .bdrv_co_create callback...
WARNING: line over 80 characters
#216: FILE: block/vmdk.c:2075:
+ bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file,
WARNING: line over 80 characters
#371: FILE: block/vmdk.c:2174:
+ bool flat, bool split, bool compress,
WARNING: line over 80 characters
#389: FILE: block/vmdk.c:2192:
+ rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix);
WARNING: line over 80 characters
#406: FILE: block/vmdk.c:2209:
+static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts,
total: 0 errors, 4 warnings, 647 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
Checking PATCH 3/4: iotests: Filter cid numbers in VMDK extent info... Checking PATCH 4/4: iotests: Add VMDK tests for blockdev-create...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #11:
new file mode 100755
ERROR: trailing whitespace
#250: FILE: tests/qemu-iotests/237.out:28:
+ format: $
ERROR: trailing whitespace
#277: FILE: tests/qemu-iotests/237.out:55:
+ format: $
ERROR: trailing whitespace
#304: FILE: tests/qemu-iotests/237.out:82:
+ format: $
total: 3 errors, 1 warnings, 514 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/[email protected]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]