| Message ID | 20181206151304.8388-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vmdk: Implement blockdev-create | expand |
This is a reasonably careful review of the QAPI-related parts, but more of an eye-over for the remainder. Kevin Wolf <kwolf@redhat.com> writes: > From: Fam Zheng <famz@redhat.com> > > 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. > > Signed-off-by: Fam Zheng <famz@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-core.json | 70 +++++++ > qapi/qapi-schema.json | 1 + > block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ > 3 files changed, 396 insertions(+), 127 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index d4fe710836..4778f88dd8 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -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'] } 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. > + > +## > +# @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. > + > +## > +# @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 > +# 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? > +# @subformat The subformat of the VMDK image. Default: "monolithicsparse". 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". Default: "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' } } @zeroed-grain is undocumented. > + > + > ## > # @SheepdogRedundancyType: > # > @@ -4215,6 +4284,7 @@ > 'ssh': 'BlockdevCreateOptionsSsh', > 'vdi': 'BlockdevCreateOptionsVdi', > 'vhdx': 'BlockdevCreateOptionsVhdx', > + 'vmdk': 'BlockdevCreateOptionsVmdk', > 'vpc': 'BlockdevCreateOptionsVpc' > } } > > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json > index 65b6dc2f6f..78e8bcd561 100644 > --- a/qapi/qapi-schema.json > +++ b/qapi/qapi-schema.json > @@ -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 > diff --git a/block/vmdk.c b/block/vmdk.c > index 32fc2c84b3..16f86457d7 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -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); > + > +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. > + 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"; > } > > - 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? > - 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"); [...]
Am 07.12.2018 um 08:10 hat Markus Armbruster geschrieben: > This is a reasonably careful review of the QAPI-related parts, but more > of an eye-over for the remainder. > > Kevin Wolf <kwolf@redhat.com> writes: > > > From: Fam Zheng <famz@redhat.com> > > > > 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. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > qapi/block-core.json | 70 +++++++ > > qapi/qapi-schema.json | 1 + > > block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ > > 3 files changed, 396 insertions(+), 127 deletions(-) > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > index d4fe710836..4778f88dd8 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -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'] } > > 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. 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. > > + > > +## > > +# @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. 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. > > + > > +## > > +# @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 > > > +# 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. > > +# @subformat The subformat of the VMDK image. Default: "monolithicsparse". > > 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". > > Default: "4" Fixed all the spelling changes. > > +# @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' } } > > @zeroed-grain is undocumented. It's right there at the beginning of the quote after your last comment? > > + > > + > > ## > > # @SheepdogRedundancyType: > > # > > @@ -4215,6 +4284,7 @@ > > 'ssh': 'BlockdevCreateOptionsSsh', > > 'vdi': 'BlockdevCreateOptionsVdi', > > 'vhdx': 'BlockdevCreateOptionsVhdx', > > + 'vmdk': 'BlockdevCreateOptionsVmdk', > > 'vpc': 'BlockdevCreateOptionsVpc' > > } } > > > > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json > > index 65b6dc2f6f..78e8bcd561 100644 > > --- a/qapi/qapi-schema.json > > +++ b/qapi/qapi-schema.json > > @@ -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 > > diff --git a/block/vmdk.c b/block/vmdk.c > > index 32fc2c84b3..16f86457d7 100644 > > --- a/block/vmdk.c > > +++ b/block/vmdk.c > > @@ -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); > > + > > +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. > > + 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"; > > } > > > > - 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
Kevin Wolf <kwolf@redhat.com> writes: > Am 07.12.2018 um 08:10 hat Markus Armbruster geschrieben: >> This is a reasonably careful review of the QAPI-related parts, but more >> of an eye-over for the remainder. >> >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > From: Fam Zheng <famz@redhat.com> >> > >> > 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. >> > >> > Signed-off-by: Fam Zheng <famz@redhat.com> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> > --- >> > qapi/block-core.json | 70 +++++++ >> > qapi/qapi-schema.json | 1 + >> > block/vmdk.c | 452 ++++++++++++++++++++++++++++++------------ >> > 3 files changed, 396 insertions(+), 127 deletions(-) >> > >> > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > index d4fe710836..4778f88dd8 100644 >> > --- a/qapi/block-core.json >> > +++ b/qapi/block-core.json >> > @@ -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'] } >> >> 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. > > 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. >> > + >> > +## >> > +# @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. > > 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. >> > + >> > +## >> > +# @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 >> >> > +# 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. >> > +# @subformat The subformat of the VMDK image. Default: "monolithicsparse". >> >> 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". >> >> Default: "4" > > Fixed all the spelling changes. > >> > +# @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' } } >> >> @zeroed-grain is undocumented. > > It's right there at the beginning of the quote after your last comment? You're right, sorry for the noise. >> > + >> > + >> > ## >> > # @SheepdogRedundancyType: >> > # >> > @@ -4215,6 +4284,7 @@ >> > 'ssh': 'BlockdevCreateOptionsSsh', >> > 'vdi': 'BlockdevCreateOptionsVdi', >> > 'vhdx': 'BlockdevCreateOptionsVhdx', >> > + 'vmdk': 'BlockdevCreateOptionsVmdk', >> > 'vpc': 'BlockdevCreateOptionsVpc' >> > } } >> > >> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json >> > index 65b6dc2f6f..78e8bcd561 100644 >> > --- a/qapi/qapi-schema.json >> > +++ b/qapi/qapi-schema.json >> > @@ -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 >> > diff --git a/block/vmdk.c b/block/vmdk.c >> > index 32fc2c84b3..16f86457d7 100644 >> > --- a/block/vmdk.c >> > +++ b/block/vmdk.c >> > @@ -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); >> > + >> > +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. > >> > + 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"; >> > } >> > >> > - 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. I suspect it's a remnant of v1, where Fam used all lower case enum values, plus code to match them case insensitively. > 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.
diff --git a/qapi/block-core.json b/qapi/block-core.json index d4fe710836..4778f88dd8 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -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' } } diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json index 65b6dc2f6f..78e8bcd561 100644 --- a/qapi/qapi-schema.json +++ b/qapi/qapi-schema.json @@ -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 diff --git a/block/vmdk.c b/block/vmdk.c index 32fc2c84b3..16f86457d7 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -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); + +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); + 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"; } - 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; - } - 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"); @@ -2058,10 +2045,34 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts ret = -ENOTSUP; goto exit; } + + /* Create extents */ + if (split) { + extent_size = split_size; + } else { + extent_size = size; + } + if (!split && !flat) { + created_size = extent_size; + } else { + created_size = 0; + } + /* Get the descriptor file BDS */ + blk = extent_fn(created_size, 0, flat, split, compress, zeroed_grain, + opaque, errp); + if (!blk) { + ret = -EIO; + goto exit; + } + if (!split && !flat) { + vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, created_size, + blk_bs(blk)->filename); + } + if (backing_file) { - BlockBackend *blk; + BlockBackend *backing; char *full_backing = g_new0(char, PATH_MAX); - bdrv_get_full_backing_filename_from_filename(filename, backing_file, + bdrv_get_full_backing_filename_from_filename(blk_bs(blk)->filename, backing_file, full_backing, PATH_MAX, &local_err); if (local_err) { @@ -2071,106 +2082,215 @@ static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts goto exit; } - blk = blk_new_open(full_backing, NULL, NULL, - BDRV_O_NO_BACKING, errp); + backing = blk_new_open(full_backing, NULL, NULL, + BDRV_O_NO_BACKING, errp); g_free(full_backing); - if (blk == NULL) { + if (backing == NULL) { ret = -EIO; goto exit; } - if (strcmp(blk_bs(blk)->drv->format_name, "vmdk")) { - blk_unref(blk); + if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) { + error_setg(errp, "Invalid backing file format: %s. Must be vmdk", + blk_bs(backing)->drv->format_name); + blk_unref(backing); ret = -EINVAL; goto exit; } - ret = vmdk_read_cid(blk_bs(blk), 0, &parent_cid); - blk_unref(blk); + ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid); + blk_unref(backing); if (ret) { + error_setg(errp, "Failed to read parent CID"); goto exit; } snprintf(parent_desc_line, BUF_SIZE, "parentFileNameHint=\"%s\"", backing_file); } - - /* Create extents */ - filesize = total_size; - while (filesize > 0) { - int64_t size = filesize; - - if (split && size > split_size) { - size = split_size; - } - if (split) { - snprintf(desc_filename, PATH_MAX, "%s-%c%03d%s", - prefix, flat ? 'f' : 's', ++idx, postfix); - } else if (flat) { - snprintf(desc_filename, PATH_MAX, "%s-flat%s", prefix, postfix); - } else { - snprintf(desc_filename, PATH_MAX, "%s%s", prefix, postfix); - } - snprintf(ext_filename, PATH_MAX, "%s%s", path, desc_filename); - - if (vmdk_create_extent(ext_filename, size, - flat, compress, zeroed_grain, NULL, opts, errp)) { + extent_idx = 1; + while (created_size < size) { + BlockBackend *extent_blk; + int64_t cur_size = MIN(size - created_size, extent_size); + extent_blk = extent_fn(cur_size, extent_idx, flat, split, compress, + zeroed_grain, opaque, errp); + if (!extent_blk) { ret = -EINVAL; goto exit; } - filesize -= size; - - /* Format description line */ - snprintf(desc_line, BUF_SIZE, - desc_extent_line, size / BDRV_SECTOR_SIZE, desc_filename); - g_string_append(ext_desc_lines, desc_line); + vmdk_desc_add_extent(ext_desc_lines, extent_line_fmt, cur_size, + blk_bs(extent_blk)->filename); + created_size += cur_size; + extent_idx++; + blk_unref(extent_blk); } /* generate descriptor file */ desc = g_strdup_printf(desc_template, g_random_int(), parent_cid, - fmt, + BlockdevVmdkSubformat_str(subformat), parent_desc_line, ext_desc_lines->str, hw_version, - total_size / + size / (int64_t)(63 * number_heads * BDRV_SECTOR_SIZE), number_heads, - adapter_type); + BlockdevVmdkAdapterType_str(adapter_type)); desc_len = strlen(desc); /* the descriptor offset = 0x200 */ if (!split && !flat) { desc_offset = 0x200; - } else { - ret = bdrv_create_file(filename, opts, &local_err); + } + + ret = blk_pwrite(blk, desc_offset, desc, desc_len, 0); + if (ret < 0) { + error_setg_errno(errp, -ret, "Could not write description"); + goto exit; + } + /* bdrv_pwrite write padding zeros to align to sector, we don't need that + * for description file */ + if (desc_offset == 0) { + ret = blk_truncate(blk, desc_len, PREALLOC_MODE_OFF, errp); if (ret < 0) { - error_propagate(errp, local_err); goto exit; } } + ret = 0; +exit: + if (blk) { + blk_unref(blk); + } + g_free(desc); + g_free(parent_desc_line); + g_string_free(ext_desc_lines, true); + return ret; +} - new_blk = blk_new_open(filename, NULL, NULL, - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, - &local_err); - if (new_blk == NULL) { - error_propagate(errp, local_err); - ret = -EIO; +typedef struct { + char *path; + char *prefix; + char *postfix; + QemuOpts *opts; +} VMDKCreateOptsData; + +static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx, + bool flat, bool split, bool compress, + bool zeroed_grain, void *opaque, + Error **errp) +{ + BlockBackend *blk = NULL; + BlockDriverState *bs = NULL; + VMDKCreateOptsData *data = opaque; + char *ext_filename = NULL; + char *rel_filename = NULL; + + if (idx == 0) { + rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix); + } else if (split) { + rel_filename = g_strdup_printf("%s-%c%03d%s", + data->prefix, + flat ? 'f' : 's', idx, data->postfix); + } else { + assert(idx == 1); + rel_filename = g_strdup_printf("%s-flat%s", data->prefix, data->postfix); + } + + ext_filename = g_strdup_printf("%s%s", data->path, rel_filename); + g_free(rel_filename); + + if (vmdk_create_extent(ext_filename, size, + flat, compress, zeroed_grain, &blk, data->opts, + errp)) { goto exit; } + bdrv_unref(bs); +exit: + g_free(ext_filename); + return blk; +} - blk_set_allow_write_beyond_eof(new_blk, true); +static int coroutine_fn vmdk_co_create_opts(const char *filename, QemuOpts *opts, + Error **errp) +{ + Error *local_err = NULL; + char *desc = NULL; + int64_t total_size = 0; + char *adapter_type = NULL; + BlockdevVmdkAdapterType adapter_type_enum; + char *backing_file = NULL; + char *hw_version = NULL; + char *fmt = NULL; + BlockdevVmdkSubformat subformat; + int ret = 0; + 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); + char *parent_desc_line = g_malloc0(BUF_SIZE); + bool zeroed_grain; + bool compat6; + int i; + VMDKCreateOptsData data; - ret = blk_pwrite(new_blk, desc_offset, desc, desc_len, 0); - if (ret < 0) { - error_setg_errno(errp, -ret, "Could not write description"); + if (filename_decompose(filename, path, prefix, postfix, PATH_MAX, errp)) { + ret = -EINVAL; goto exit; } - /* bdrv_pwrite write padding zeros to align to sector, we don't need that - * for description file */ - if (desc_offset == 0) { - ret = blk_truncate(new_blk, desc_len, PREALLOC_MODE_OFF, errp); + /* 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); + compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false); + if (strcmp(hw_version, "undefined") == 0) { + g_free(hw_version); + hw_version = g_strdup("4"); } -exit: - if (new_blk) { - blk_unref(new_blk); + fmt = qemu_opt_get_del(opts, BLOCK_OPT_SUBFMT); + zeroed_grain = qemu_opt_get_bool_del(opts, BLOCK_OPT_ZEROED_GRAIN, false); + + if (adapter_type) { + for (i = 0; i < strlen(adapter_type); ++i) { + adapter_type[i] = qemu_tolower(adapter_type[i]); + } + adapter_type_enum = qapi_enum_parse(&BlockdevVmdkAdapterType_lookup, + adapter_type, + BLOCKDEV_VMDK_ADAPTER_TYPE_IDE, + &local_err); + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto exit; + } + } else { + adapter_type_enum = BLOCKDEV_VMDK_ADAPTER_TYPE_IDE; } + + if (!fmt) { + /* Default format to monolithicSparse */ + subformat = BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE; + } else { + subformat = qapi_enum_parse(&BlockdevVmdkSubformat_lookup, + fmt, + BLOCKDEV_VMDK_SUBFORMAT_MONOLITHICSPARSE, + &local_err); + if (local_err) { + error_propagate(errp, local_err); + ret = -EINVAL; + goto exit; + } + } + data = (VMDKCreateOptsData){ + .prefix = prefix, + .postfix = postfix, + .path = path, + .opts = opts, + }; + ret = vmdk_co_do_create(total_size, subformat, adapter_type_enum, + backing_file, hw_version, compat6, zeroed_grain, + vmdk_co_create_opts_cb, &data, errp); + +exit: g_free(adapter_type); g_free(backing_file); g_free(hw_version); @@ -2183,7 +2303,84 @@ exit: g_free(ext_filename); g_free(desc_filename); g_free(parent_desc_line); - g_string_free(ext_desc_lines, true); + return ret; +} + +static BlockBackend *vmdk_co_create_cb(int64_t size, int idx, + bool flat, bool split, bool compress, + bool zeroed_grain, void *opaque, + Error **errp) +{ + int ret; + BlockDriverState *bs; + BlockBackend *blk; + BlockdevCreateOptionsVmdk *opts = opaque; + + if (idx == 0) { + bs = bdrv_open_blockdev_ref(opts->file, errp); + } else { + int i; + BlockdevRefList *list = opts->extents; + for (i = 1; i < idx; i++) { + if (!list || !list->next) { + error_setg(errp, "Extent [%d] not specified", i); + return NULL; + } + list = list->next; + } + if (!list) { + error_setg(errp, "Extent [%d] not specified", idx - 1); + return NULL; + } + bs = bdrv_open_blockdev_ref(list->value, errp); + } + if (!bs) { + return NULL; + } + blk = blk_new(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE, + BLK_PERM_ALL); + if (blk_insert_bs(blk, bs, errp)) { + bdrv_unref(bs); + return NULL; + } + blk_set_allow_write_beyond_eof(blk, true); + bdrv_unref(bs); + + ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp); + if (ret) { + blk_unref(blk); + blk = NULL; + } + return blk; +} + +static int coroutine_fn vmdk_co_create(BlockdevCreateOptions *create_options, + Error **errp) +{ + int ret; + BlockdevCreateOptionsVmdk *opts; + + opts = &create_options->u.vmdk; + + /* Validate options */ + if (!QEMU_IS_ALIGNED(opts->size, BDRV_SECTOR_SIZE)) { + error_setg(errp, "Image size must be a multiple of 512 bytes"); + ret = -EINVAL; + goto out; + } + + ret = vmdk_co_do_create(opts->size, + opts->subformat, + opts->adapter_type, + opts->backing_file, + opts->hwversion, + false, + opts->zeroed_grain, + vmdk_co_create_cb, + opts, errp); + return ret; + +out: return ret; } @@ -2451,6 +2648,7 @@ static BlockDriver bdrv_vmdk = { .bdrv_co_pwrite_zeroes = vmdk_co_pwrite_zeroes, .bdrv_close = vmdk_close, .bdrv_co_create_opts = vmdk_co_create_opts, + .bdrv_co_create = vmdk_co_create, .bdrv_co_flush_to_disk = vmdk_co_flush, .bdrv_co_block_status = vmdk_co_block_status, .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,