Re: [Qemu-devel] [PATCH v14 3/5] qapi: introduce PreallocMode and new PreallocModes full and falloc.

["Benoît Canet" <benoit.canet@irqsave.net>]

The Tuesday 09 Sep 2014 à 11:54:29 (+0800), Hu Tao wrote :
> This patch prepares for the subsequent patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c              | 23 +++++++++++++++--------
>  qapi/block-core.json       | 17 +++++++++++++++++
>  tests/qemu-iotests/049.out |  2 +-
>  3 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cf27c3f..94d1225 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -30,6 +30,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qbool.h"
> +#include "qapi/util.h"
>  #include "trace.h"
>  #include "qemu/option_int.h"
>  
> @@ -1738,7 +1739,7 @@ static int preallocate(BlockDriverState *bs)
>  
>  static int qcow2_create2(const char *filename, int64_t total_size,
>                           const char *backing_file, const char *backing_format,
> -                         int flags, size_t cluster_size, int prealloc,
> +                         int flags, size_t cluster_size, PreallocMode prealloc,
>                           QemuOpts *opts, int version,
>                           Error **errp)

below that.

Carefull study of the code tell us that here prealloc will be 0 or 1
but i think you could prepare a bit sooner the next patch by doing:

-    if (prealloc) {                                                             
+    if (prealloc == PREALLOC_MODE_METADATA) {
            BDRVQcowState *s = bs->opaque;

in the same qcow2_create2 function.


If you do so someone who start reading the code at this precise commit will
not have to lookup the declaration order of PreallocMode in the QAPI file.

>  {
> @@ -1915,7 +1916,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>      uint64_t size = 0;
>      int flags = 0;
>      size_t cluster_size = DEFAULT_CLUSTER_SIZE;
> -    int prealloc = 0;
> +    PreallocMode prealloc;
>      int version = 3;
>      Error *local_err = NULL;
>      int ret;
> @@ -1931,12 +1932,11 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>      cluster_size = qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE,
>                                           DEFAULT_CLUSTER_SIZE);
>      buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> -    if (!buf || !strcmp(buf, "off")) {
> -        prealloc = 0;
> -    } else if (!strcmp(buf, "metadata")) {
> -        prealloc = 1;
> -    } else {
> -        error_setg(errp, "Invalid preallocation mode: '%s'", buf);
> +    prealloc = qapi_enum_parse(PreallocMode_lookup, buf,
> +                               PREALLOC_MODE_MAX, PREALLOC_MODE_OFF,
> +                               &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>          ret = -EINVAL;
>          goto finish;
>      }
> @@ -1958,6 +1958,13 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
>          flags |= BLOCK_FLAG_LAZY_REFCOUNTS;
>      }
>  
> +    if (prealloc && prealloc != PREALLOC_MODE_METADATA) {
> +        ret = -EINVAL;
> +        error_setg(errp, "Unsupported preallocate mode: %s",
> +                   PreallocMode_lookup[prealloc]);
> +        goto finish;
> +    }
> +
>      if (backing_file && prealloc) {
>          error_setg(errp, "Backing file and preallocation cannot be used at "
>                     "the same time");
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index a685d02..a29dbe1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1697,3 +1697,20 @@
>              'len'   : 'int',
>              'offset': 'int',
>              'speed' : 'int' } }
> +
> +# @PreallocMode
> +#
> +# Preallocation mode of QEMU image file
> +#
> +# @off: no preallocation
> +# @metadata: preallocate only for metadata
> +# @falloc: like @full preallocation but allocate disk space by
> +#          posix_fallocate() rather than writing zeros.
> +# @full: preallocate all data by writing zeros to device to ensure disk
> +#        space is really available. @full preallocation also sets up
> +#        metadata correctly.
> +#
> +# Since 2.2
> +##
> +{ 'enum': 'PreallocMode',
> +  'data': [ 'off', 'metadata', 'falloc', 'full' ] }
> diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
> index 71ca44d..09ca0ae 100644
> --- a/tests/qemu-iotests/049.out
> +++ b/tests/qemu-iotests/049.out
> @@ -179,7 +179,7 @@ qemu-img create -f qcow2 -o preallocation=metadata TEST_DIR/t.qcow2 64M
>  Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off 
>  
>  qemu-img create -f qcow2 -o preallocation=1234 TEST_DIR/t.qcow2 64M
> -qemu-img: TEST_DIR/t.qcow2: Invalid preallocation mode: '1234'
> +qemu-img: TEST_DIR/t.qcow2: invalid parameter value: 1234
>  Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 preallocation='1234' lazy_refcounts=off 
>  
>  == Check encryption option ==
> -- 
> 1.9.3
> 
>