Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/4] vmdk: Implement .bdrv_co_create callback

[Markus Armbruster]

Kevin Wolf <address@hidden> 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 <address@hidden> writes:
>> 
>> > From: Fam Zheng <address@hidden>
>> >
>> > 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 <address@hidden>
>> > Signed-off-by: Kevin Wolf <address@hidden>
>> > ---
>> >  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.