diff mbox

[v13,4/6] qapi: introduce PreallocMode and a new PreallocMode full.

Message ID a2c13d50d041d36022ded9835b6e6e00dc41ac8f.1409299732.git.hutao@cn.fujitsu.com
State New
Headers show

Commit Message

Hu Tao Aug. 29, 2014, 8:33 a.m. UTC 
This patch prepares for the subsequent patches.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c              | 23 +++++++++++++++--------
 qapi/block-core.json       | 16 ++++++++++++++++
 tests/qemu-iotests/049.out |  2 +-
 3 files changed, 32 insertions(+), 9 deletions(-)

Comments

Max Reitz Sept. 2, 2014, 9:32 p.m. UTC | #1 
On 29.08.2014 10:33, Hu Tao wrote:
> This patch prepares for the subsequent patches.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c              | 23 +++++++++++++++--------
>   qapi/block-core.json       | 16 ++++++++++++++++
>   tests/qemu-iotests/049.out |  2 +-
>   3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index cf27c3f..95fb240 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)
>   {
> @@ -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 = PREALLOC_MODE_OFF;
>       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 = -1;

Since the return value is expected to be -errno, I'd propose "ret = 
-EINVAL;" here. With that fixed (or a good explanation why we want -1 here):

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +        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 fb74c56..543b00b 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1679,3 +1679,19 @@
>               'len'   : 'int',
>               'offset': 'int',
>               'speed' : 'int' } }
> +
> +# @PreallocMode
> +#
> +# Preallocation mode of QEMU image file
> +#
> +# @off: no preallocation
> +# @metadata: preallocate only for metadata
> +# @full: preallocate all data by calling posix_fallocate() if it is
> +#        available, otherwise 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', '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 ==
Eric Blake Sept. 2, 2014, 9:51 p.m. UTC | #2 
On 08/29/2014 02:33 AM, Hu Tao wrote:
> This patch prepares for the subsequent patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2.c              | 23 +++++++++++++++--------
>  qapi/block-core.json       | 16 ++++++++++++++++
>  tests/qemu-iotests/049.out |  2 +-
>  3 files changed, 32 insertions(+), 9 deletions(-)
> 

> @@ -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) {

I find it a bit awkward that you are checking for PREALLOC_MODE_OFF
implicitly ('prealloc &&') vs. checking for prealloc mode METADATA
explicitly.  Since there are only three modes, would it be any simpler
to just have written:

if (prealloc == PREALLOC_MODE_FULL) {
Hu Tao Sept. 3, 2014, 1:31 a.m. UTC | #3 
On Tue, Sep 02, 2014 at 11:32:50PM +0200, Max Reitz wrote:
> On 29.08.2014 10:33, Hu Tao wrote:
> >This patch prepares for the subsequent patches.
> >
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> >  block/qcow2.c              | 23 +++++++++++++++--------
> >  qapi/block-core.json       | 16 ++++++++++++++++
> >  tests/qemu-iotests/049.out |  2 +-
> >  3 files changed, 32 insertions(+), 9 deletions(-)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index cf27c3f..95fb240 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)
> >  {
> >@@ -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 = PREALLOC_MODE_OFF;
> >      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 = -1;
> 
> Since the return value is expected to be -errno, I'd propose "ret =
> -EINVAL;" here. With that fixed (or a good explanation why we want
> -1 here):

Good.

> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> >+        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 fb74c56..543b00b 100644
> >--- a/qapi/block-core.json
> >+++ b/qapi/block-core.json
> >@@ -1679,3 +1679,19 @@
> >              'len'   : 'int',
> >              'offset': 'int',
> >              'speed' : 'int' } }
> >+
> >+# @PreallocMode
> >+#
> >+# Preallocation mode of QEMU image file
> >+#
> >+# @off: no preallocation
> >+# @metadata: preallocate only for metadata
> >+# @full: preallocate all data by calling posix_fallocate() if it is
> >+#        available, otherwise 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', '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 ==
Hu Tao Sept. 3, 2014, 1:35 a.m. UTC | #4 
On Tue, Sep 02, 2014 at 03:51:23PM -0600, Eric Blake wrote:
> On 08/29/2014 02:33 AM, Hu Tao wrote:
> > This patch prepares for the subsequent patches.
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2.c              | 23 +++++++++++++++--------
> >  qapi/block-core.json       | 16 ++++++++++++++++
> >  tests/qemu-iotests/049.out |  2 +-
> >  3 files changed, 32 insertions(+), 9 deletions(-)
> > 
> 
> > @@ -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) {

This one reads as 'support PREALLOC_MODE_METADATA' only,

> 
> I find it a bit awkward that you are checking for PREALLOC_MODE_OFF
> implicitly ('prealloc &&') vs. checking for prealloc mode METADATA
> explicitly.  Since there are only three modes, would it be any simpler
> to just have written:
> 
> if (prealloc == PREALLOC_MODE_FULL) {

and this one reads as 'does't support PREALLOC_MODE_FULL'.  Althrough
they are the same, but I'd prefer the former one. Anyway, the check is
removed in patch 6.

Regards,
Hu

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Kevin Wolf Sept. 4, 2014, 12:17 p.m. UTC | #5 
Am 29.08.2014 um 10:33 hat Hu Tao geschrieben:
> This patch prepares for the subsequent patches.
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>

> @@ -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 = PREALLOC_MODE_OFF;
>      int version = 3;
>      Error *local_err = NULL;
>      int ret;

The initialisation isn't really necessary any more, is it?

> @@ -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 = -1;
> +        error_setg(errp, "Unsupported preallocate mode: %s",
> +                   PreallocMode_lookup[prealloc]);
> +        goto finish;
> +    }
> +

Like the other reviewers, I don't like this block much. But as it
disappears later in the series and achieves it's job of maintaining
bisectability:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
diff mbox

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index cf27c3f..95fb240 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)
 {
@@ -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 = PREALLOC_MODE_OFF;
     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 = -1;
+        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 fb74c56..543b00b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1679,3 +1679,19 @@ 
             'len'   : 'int',
             'offset': 'int',
             'speed' : 'int' } }
+
+# @PreallocMode
+#
+# Preallocation mode of QEMU image file
+#
+# @off: no preallocation
+# @metadata: preallocate only for metadata
+# @full: preallocate all data by calling posix_fallocate() if it is
+#        available, otherwise 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', '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 ==