Re: [Qemu-block] [Qemu-devel] [PATCH v4 5/5] vmdk: Reject excess extents in blockdev-create

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 07.12.2018 um 14:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Clarify that the number of extents provided in BlockdevCreateOptionsVmdk
>> > must match the number of extents that will actually be used. Providing
>> > more extents will result in an error now.
>> >
>> > This requires adapting the test case to provide the right number of
>> > extents.
>> >
>> > Signed-off-by: Kevin Wolf <address@hidden>
>> > ---
>> >  qapi/block-core.json       |  3 ++-
>> >  block/vmdk.c               | 29 ++++++++++++++++++++++++-----
>> >  tests/qemu-iotests/237     |  6 +++++-
>> >  tests/qemu-iotests/237.out | 13 +++++++------
>> >  4 files changed, 38 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 0793550cf2..afdd2f89a2 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
>> > @@ -4068,7 +4068,8 @@
>> >  #               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.
>> > +#               calculated as extent_number = virtual_size / 2GB. 
>> > Providing
>> > +#               more extents than will be used is an error.
>> >  # @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.
>> > diff --git a/block/vmdk.c b/block/vmdk.c
>> > index 5a162ee85c..682ad93aa1 100644
>> > --- a/block/vmdk.c
>> > +++ b/block/vmdk.c
>> > @@ -1970,6 +1970,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t 
>> > size,
>> >  {
>> >      int extent_idx;
>> >      BlockBackend *blk = NULL;
>> > +    BlockBackend *extent_blk;
>> >      Error *local_err = NULL;
>> >      char *desc = NULL;
>> >      int ret = 0;
>> > @@ -2107,7 +2108,6 @@ static int coroutine_fn vmdk_co_do_create(int64_t 
>> > size,
>> >      }
>> >      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);
>> > @@ -2121,6 +2121,17 @@ static int coroutine_fn vmdk_co_do_create(int64_t 
>> > size,
>> >          extent_idx++;
>> >          blk_unref(extent_blk);
>> >      }
>> > +
>> > +    /* Check whether we got excess extents */
>> > +    extent_blk = extent_fn(-1, extent_idx, flat, split, compress, 
>> > zeroed_grain,
>> > +                           opaque, NULL);
>> > +    if (extent_blk) {
>> > +        blk_unref(extent_blk);
>> > +        error_setg(errp, "List of extents contains unused extents");
>> > +        ret = -EINVAL;
>> > +        goto exit;
>> > +    }
>> > +
>> >      /* generate descriptor file */
>> >      desc = g_strdup_printf(desc_template,
>> >                             g_random_int(),
>> > @@ -2181,6 +2192,12 @@ static BlockBackend *vmdk_co_create_opts_cb(int64_t 
>> > size, int idx,
>> >      char *ext_filename = NULL;
>> >      char *rel_filename = NULL;
>> >  
>> > +    /* We're done, don't create excess extents. */
>> > +    if (size == -1) {
>> > +        assert(errp == NULL);
>> > +        return NULL;
>> > +    }
>> > +
>> 
>> I'm afraid I don't get this hunk.
>
> vmdk_co_create_opts_cb() is for the legacy code path, where 'qemu-img
> create' passes us a QemuOpts. This doesn't contain extents, so rather
> than returning ther next extent from the input, this function simply
> creates a new extent file each time it is called.
>
> When checking whether we have too many extents, we don't know from which
> code path we came, so we have to call the callback uncondtionally and
> see whether it still returns an extra extent. In this case, creating a
> new one would obviously be counterproductive and trigger an error, so I
> just return NULL immediately.
>
> If you have a suggestion for a comment to put somewhere, I'll gladly add
> it.

Coming up with intelligble comments exceed my mental capabilities right
now, I'm afraid.  But I can still give my
Reviewed-by: Markus Armbruster <address@hidden>