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

[Markus Armbruster]

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.

>      if (idx == 0) {
>          rel_filename = g_strdup_printf("%s%s", data->prefix, data->postfix);
>      } else if (split) {
> @@ -2342,10 +2359,12 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, 
> int idx,
>      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;
> +    if (size != -1) {
> +        ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, 
> errp);
> +        if (ret) {
> +            blk_unref(blk);
> +            blk = NULL;
> +        }
>      }
>      return blk;
>  }
> diff --git a/tests/qemu-iotests/237 b/tests/qemu-iotests/237
> index e04a1ac6be..251771d7fb 100755
> --- a/tests/qemu-iotests/237
> +++ b/tests/qemu-iotests/237
> @@ -20,6 +20,7 @@
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  #
>  
> +import math
>  import iotests
>  from iotests import imgfmt
>  
> @@ -222,12 +223,15 @@ with iotests.FilePath('t.vmdk') as disk_path, \
>              iotests.log("= %s %d =" % (subfmt, size))
>              iotests.log("")
>  
> +            num_extents = math.ceil(size / 2.0**31)
> +            extents = [ "ext%d" % (i) for i in range(1, num_extents + 1) ]
> +
>              vm.launch()
>              blockdev_create(vm, { 'driver': imgfmt,
>                                    'file': 'node0',
>                                    'size': size,
>                                    'subformat': subfmt,
> -                                  'extents': ['ext1', 'ext2', 'ext3'] })
> +                                  'extents': extents })
>              vm.shutdown()
>  
>              iotests.img_info_log(disk_path)
> diff --git a/tests/qemu-iotests/237.out b/tests/qemu-iotests/237.out
> index 1aa9aad349..241c864369 100644
> --- a/tests/qemu-iotests/237.out
> +++ b/tests/qemu-iotests/237.out
> @@ -154,6 +154,7 @@ Job failed: Extent [0] not specified
>  
>  {"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 512, "subformat": "monolithicFlat"}}}
>  {"return": {}}
> +Job failed: List of extents contains unused extents
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
>  
> @@ -161,7 +162,7 @@ Job failed: Extent [0] not specified
>  
>  = twoGbMaxExtentFlat 512 =
>  
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 512, "subformat": "twoGbMaxExtentFlat"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, 
> "subformat": "twoGbMaxExtentFlat"}}}
>  {"return": {}}
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
> @@ -181,7 +182,7 @@ Format specific information:
>  
>  = twoGbMaxExtentSparse 512 =
>  
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 512, "subformat": "twoGbMaxExtentSparse"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 512, 
> "subformat": "twoGbMaxExtentSparse"}}}
>  {"return": {}}
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
> @@ -203,7 +204,7 @@ Format specific information:
>  
>  = twoGbMaxExtentFlat 1073741824 =
>  
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 1073741824, "subformat": "twoGbMaxExtentFlat"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, 
> "subformat": "twoGbMaxExtentFlat"}}}
>  {"return": {}}
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
> @@ -223,7 +224,7 @@ Format specific information:
>  
>  = twoGbMaxExtentSparse 1073741824 =
>  
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 1073741824, "subformat": "twoGbMaxExtentSparse"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 1073741824, 
> "subformat": "twoGbMaxExtentSparse"}}}
>  {"return": {}}
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
> @@ -245,7 +246,7 @@ Format specific information:
>  
>  = twoGbMaxExtentFlat 2147483648 =
>  
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 2147483648, "subformat": "twoGbMaxExtentFlat"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, 
> "subformat": "twoGbMaxExtentFlat"}}}
>  {"return": {}}
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}
> @@ -265,7 +266,7 @@ Format specific information:
>  
>  = twoGbMaxExtentSparse 2147483648 =
>  
> -{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1", "ext2", "ext3"], "file": "node0", 
> "size": 2147483648, "subformat": "twoGbMaxExtentSparse"}}}
> +{"execute": "blockdev-create", "arguments": {"job_id": "job0", "options": 
> {"driver": "vmdk", "extents": ["ext1"], "file": "node0", "size": 2147483648, 
> "subformat": "twoGbMaxExtentSparse"}}}
>  {"return": {}}
>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>  {"return": {}}

Apart from the hunk that stumps me, the patch looks good to me.