Kevin Wolf <kw...@redhat.com> writes:
> Am 07.12.2018 um 14:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@redhat.com> 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 <kw...@redhat.com>
>> > ---
>> > 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 <arm...@redhat.com>
