diff mbox

[14/34] qcow2: Factor out qcow2_update_options()

Message ID 1431105726-3682-15-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf May 8, 2015, 5:21 p.m. UTC 
Eventually we want to be able to change options at runtime. As a first
step towards that goal, separate some option handling code from the
general initialisation code in qcow2_open().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 76 insertions(+), 59 deletions(-)

Comments

Eric Blake May 12, 2015, 8:04 p.m. UTC | #1 
On 05/08/2015 11:21 AM, Kevin Wolf wrote:
> Eventually we want to be able to change options at runtime. As a first
> step towards that goal, separate some option handling code from the
> general initialisation code in qcow2_open().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 76 insertions(+), 59 deletions(-)
> 
> +    } else {
> +        error_setg(errp, "Unsupported value '%s' for qcow2 option "
> +                   "'overlap-check'. Allowed are either of the following: "
> +                   "none, constant, cached, all", opt_overlap_check);

Pre-existing due to code motion, but I find s/either/any/ easier to read.

Reviewed-by: Eric Blake <eblake@redhat.com>
Kevin Wolf May 13, 2015, 9:11 a.m. UTC | #2 
Am 12.05.2015 um 22:04 hat Eric Blake geschrieben:
> On 05/08/2015 11:21 AM, Kevin Wolf wrote:
> > Eventually we want to be able to change options at runtime. As a first
> > step towards that goal, separate some option handling code from the
> > general initialisation code in qcow2_open().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 76 insertions(+), 59 deletions(-)
> > 
> > +    } else {
> > +        error_setg(errp, "Unsupported value '%s' for qcow2 option "
> > +                   "'overlap-check'. Allowed are either of the following: "
> > +                   "none, constant, cached, all", opt_overlap_check);
> 
> Pre-existing due to code motion, but I find s/either/any/ easier to read.

Isn't "either" only for a choice between two things anyway?

The series isn't long enough yet, I'll fix it. :-)

Kevin
Max Reitz May 13, 2015, 11:21 a.m. UTC | #3 
On 08.05.2015 19:21, Kevin Wolf wrote:
> Eventually we want to be able to change options at runtime. As a first
> step towards that goal, separate some option handling code from the
> general initialisation code in qcow2_open().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 76 insertions(+), 59 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b9a72e3..db535d4 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[snip]

> @@ -549,8 +623,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       Error *local_err = NULL;
>       uint64_t ext_end;
>       uint64_t l1_vm_state_index;
> -    const char *opt_overlap_check, *opt_overlap_check_template;
> -    int overlap_check_template = 0;
>       uint64_t l2_cache_size, refcount_cache_size;
>   
>       ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
> @@ -924,69 +996,14 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>       }
>   
>       /* Enable lazy_refcounts according to image and command line options */

[...]

> +    ret = qcow2_update_options(bs, opts, flags, errp);
> +    if (ret < 0) {

The comment looks a bit strange now, because qcow2_update_options() 
doesn't update just lazy_refcounts.

Max
Max Reitz May 13, 2015, 11:28 a.m. UTC | #4 
On 08.05.2015 19:21, Kevin Wolf wrote:
> Eventually we want to be able to change options at runtime. As a first
> step towards that goal, separate some option handling code from the
> general initialisation code in qcow2_open().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
>   1 file changed, 76 insertions(+), 59 deletions(-)

Now seeing that the comment I was complaining about is removed in patch 16:

Reviewed-by: Max Reitz <mreitz@redhat.com>
Eric Blake May 13, 2015, 5:04 p.m. UTC | #5 
On 05/13/2015 03:11 AM, Kevin Wolf wrote:
> Am 12.05.2015 um 22:04 hat Eric Blake geschrieben:
>> On 05/08/2015 11:21 AM, Kevin Wolf wrote:
>>> Eventually we want to be able to change options at runtime. As a first
>>> step towards that goal, separate some option handling code from the
>>> general initialisation code in qcow2_open().
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
>>>  1 file changed, 76 insertions(+), 59 deletions(-)
>>>
>>> +    } else {
>>> +        error_setg(errp, "Unsupported value '%s' for qcow2 option "
>>> +                   "'overlap-check'. Allowed are either of the following: "
>>> +                   "none, constant, cached, all", opt_overlap_check);
>>
>> Pre-existing due to code motion, but I find s/either/any/ easier to read.
> 
> Isn't "either" only for a choice between two things anyway?

Exactly. And since there are four things, that's why I found it easier
to read.

> 
> The series isn't long enough yet, I'll fix it. :-)

If you want conciseness, this would also work:

"Unsupported value '%s' for qcow2 option 'overlap-check'; expecting one
of: none, constant, cached, all"

or even omitting the list of valid options altogether (which in the long
run is easier to maintain if we anticipate extending the list - as there
are fewer places where copies of the list need to be kept in sync)
diff mbox

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index b9a72e3..db535d4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -538,6 +538,80 @@  static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size,
     }
 }
 
+static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+                                int flags, Error **errp)
+{
+    BDRVQcowState *s = bs->opaque;
+    const char *opt_overlap_check, *opt_overlap_check_template;
+    int overlap_check_template = 0;
+    int i;
+    int ret;
+
+    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+        (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+
+    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+                          flags & BDRV_O_UNMAP);
+    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
+
+    opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
+    opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
+    if (opt_overlap_check_template && opt_overlap_check &&
+        strcmp(opt_overlap_check_template, opt_overlap_check))
+    {
+        error_setg(errp, "Conflicting values for qcow2 options '"
+                   QCOW2_OPT_OVERLAP "' ('%s') and '" QCOW2_OPT_OVERLAP_TEMPLATE
+                   "' ('%s')", opt_overlap_check, opt_overlap_check_template);
+        ret = -EINVAL;
+        goto fail;
+    }
+    if (!opt_overlap_check) {
+        opt_overlap_check = opt_overlap_check_template ?: "cached";
+    }
+
+    if (!strcmp(opt_overlap_check, "none")) {
+        overlap_check_template = 0;
+    } else if (!strcmp(opt_overlap_check, "constant")) {
+        overlap_check_template = QCOW2_OL_CONSTANT;
+    } else if (!strcmp(opt_overlap_check, "cached")) {
+        overlap_check_template = QCOW2_OL_CACHED;
+    } else if (!strcmp(opt_overlap_check, "all")) {
+        overlap_check_template = QCOW2_OL_ALL;
+    } else {
+        error_setg(errp, "Unsupported value '%s' for qcow2 option "
+                   "'overlap-check'. Allowed are either of the following: "
+                   "none, constant, cached, all", opt_overlap_check);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    s->overlap_check = 0;
+    for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
+        /* overlap-check defines a template bitmask, but every flag may be
+         * overwritten through the associated boolean option */
+        s->overlap_check |=
+            qemu_opt_get_bool(opts, overlap_bool_option_names[i],
+                              overlap_check_template & (1 << i)) << i;
+    }
+
+    if (s->use_lazy_refcounts && s->qcow_version < 3) {
+        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+                   "qemu 1.1 compatibility level");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    return ret;
+}
+
 static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
@@ -549,8 +623,6 @@  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
-    const char *opt_overlap_check, *opt_overlap_check_template;
-    int overlap_check_template = 0;
     uint64_t l2_cache_size, refcount_cache_size;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
@@ -924,69 +996,14 @@  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Enable lazy_refcounts according to image and command line options */
-    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
-        (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
-
-    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
-                          flags & BDRV_O_UNMAP);
-    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
-    opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
-    opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
-    if (opt_overlap_check_template && opt_overlap_check &&
-        strcmp(opt_overlap_check_template, opt_overlap_check))
-    {
-        error_setg(errp, "Conflicting values for qcow2 options '"
-                   QCOW2_OPT_OVERLAP "' ('%s') and '" QCOW2_OPT_OVERLAP_TEMPLATE
-                   "' ('%s')", opt_overlap_check, opt_overlap_check_template);
-        ret = -EINVAL;
-        goto fail;
-    }
-    if (!opt_overlap_check) {
-        opt_overlap_check = opt_overlap_check_template ?: "cached";
-    }
-
-    if (!strcmp(opt_overlap_check, "none")) {
-        overlap_check_template = 0;
-    } else if (!strcmp(opt_overlap_check, "constant")) {
-        overlap_check_template = QCOW2_OL_CONSTANT;
-    } else if (!strcmp(opt_overlap_check, "cached")) {
-        overlap_check_template = QCOW2_OL_CACHED;
-    } else if (!strcmp(opt_overlap_check, "all")) {
-        overlap_check_template = QCOW2_OL_ALL;
-    } else {
-        error_setg(errp, "Unsupported value '%s' for qcow2 option "
-                   "'overlap-check'. Allowed are either of the following: "
-                   "none, constant, cached, all", opt_overlap_check);
-        ret = -EINVAL;
+    ret = qcow2_update_options(bs, opts, flags, errp);
+    if (ret < 0) {
         goto fail;
     }
 
-    s->overlap_check = 0;
-    for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
-        /* overlap-check defines a template bitmask, but every flag may be
-         * overwritten through the associated boolean option */
-        s->overlap_check |=
-            qemu_opt_get_bool(opts, overlap_bool_option_names[i],
-                              overlap_check_template & (1 << i)) << i;
-    }
-
     qemu_opts_del(opts);
     opts = NULL;
 
-    if (s->use_lazy_refcounts && s->qcow_version < 3) {
-        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
-                   "qemu 1.1 compatibility level");
-        ret = -EINVAL;
-        goto fail;
-    }
-
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};