[v14,3/5] qapi: introduce PreallocMode and new PreallocModes full and falloc.

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(-)

On 09/08/2014 09:54 PM, 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(-)
> 

>      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;
> +    }

I _still_ think this looks weird, and would be better as either:

if (prealloc != PREALLOC_MODE_NONE &&
    prealloc != PREALLOC_MODE_METADATA) {

to make it obvious that you are filtering for two acceptable modes, or as:

if (prealloc == PREALLOC_MODE_FALLOC ||
    prealloc == PREALLOC_MODE_FULL) {

to make it obvious the modes that you do not support.  But my complaint
is not strong enough to prevent this patch, especially if later in the
series revisits this code.

Reviewed-by: Eric Blake <eblake@redhat.com>

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
> 
>

On Tue, Sep 09, 2014 at 02:45:56PM +0200, Benoît Canet wrote:
> 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.

Thanks for your suggestion, I think this makes much sense.

Regards,
Hu

On Tue, Sep 09, 2014 at 06:42:13AM -0600, Eric Blake wrote:
> On 09/08/2014 09:54 PM, 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(-)
> > 
> 
> >      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;
> > +    }
> 
> I _still_ think this looks weird, and would be better as either:
> 
> if (prealloc != PREALLOC_MODE_NONE &&
>     prealloc != PREALLOC_MODE_METADATA) {
> 
> to make it obvious that you are filtering for two acceptable modes, or as:
> 
> if (prealloc == PREALLOC_MODE_FALLOC ||
>     prealloc == PREALLOC_MODE_FULL) {
> 
> to make it obvious the modes that you do not support.  But my complaint
> is not strong enough to prevent this patch, especially if later in the
> series revisits this code.

After reading Benoît's comment, I understand why you think the code
looks weird. I'll change it as you suggested. Thanks! 

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>