Re: [Qemu-devel] [PATCH 2/3] qcow2: Implement bdrv_amend_options

[Kevin Wolf]

Am 28.08.2013 um 14:05 hat Max Reitz geschrieben:
> Hi,
> 
> Am 28.08.2013 13:48, schrieb Kevin Wolf:
> >Am 28.08.2013 um 13:39 hat Max Reitz geschrieben:
> >>>>+    /* clear incompatible features */
> >>>>+    if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
> >>>>+        BdrvCheckResult result;
> >>>>+        ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
> >>>>+        if (ret < 0) {
> >>>>+            return ret;
> >>>>+        }
> >>>This is unnecessary: The image could be opened, so we know that it was
> >>>clean when we started. We also know that we haven't crashed yet, so if we
> >>>flush all in-memory data, we'll have a consistent on-disk state again.
> >>>
> >>>qcow2_mark_clean() already calls bdrv_flush(bs), so it does everything
> >>>that is needed in this respect.
> >>So that flag should always be already cleared at this point anyway?
> >The qcow2_mark_clean() call is on the next line (which you removed from
> >the quote), so not at this point but one line later. But yeah, it's done
> >by other code.
> Yes, I was referring to other code (which cleans the image right at
> opening).

Well, yes and no then.

Yes, qcow2_open() checks the dirty flag and when it's set, it repairs
the image, which in turn clears the dirty flag.

No, this does not mean that the flag is clear at this point. The next
cluster allocation makes the image dirty again, but we have all
information about the cluster allocation in memory, so a simple flush
makes the image consistent. That's why qcow2_mark_clean() must be called
here, and why calling only this is sufficient.

> >>>>+        } else if (!strcmp(options[i].name, "encryption")) {
> >>>>+            if (options[i].value.n != !!s->crypt_method) {
> >>>>+                fprintf(stderr, "Changing the encryption flag is not "
> >>>>+                        "supported.\n");
> >>>>+                return -ENOTSUP;
> >>>>+            }
> >>>>+        } else if (!strcmp(options[i].name, "cluster_size")) {
> >>>>+            if (options[i].value.n && (options[i].value.n != 
> >>>>s->cluster_size)) {
> >>>>+                fprintf(stderr, "Changing the cluster size is not "
> >>>>+                        "supported.\n");
> >>>>+                return -ENOTSUP;
> >>>>+            }
> >>>>+        } else if (!strcmp(options[i].name, "lazy_refcounts")) {
> >>>>+            /* TODO: detect whether this flag was indeed explicitly 
> >>>>given */
> >>>>+            lazy_refcounts = options[i].value.n;
> >>>I can see two ways to achieve this:
> >>>
> >>>1. Add a new field 'bool assigned' to QEMUOptionParameter, which would
> >>>    be cleared before parsing an option string and set for each option in
> >>>    set_option_parameter()
> >>>
> >>>2. Get the QemuOpts conversion series in and add a function that tells
> >>>    whether a given option was specified or not.
> >>>
> >>>The same TODO should actually apply to encryption and cluster_size as
> >>>well, shouldn't it?
> >>Kind of; however, a cluster_size of 0 seems invalid to me, therefore
> >>it is pretty easy to detect that option not being given.
> >Depends on whether you think that 'qemu-img amend -o cluster_size=0' is
> >a valid way of saying that you don't want to change the cluster size. I
> >would prefer to error out.
> No, I just missed the default for that option not being zero. Sorry,
> my fault.

Doesn't really change my point: Even if the default was 0, deciding that
the option wasn't given by checking against an invalid value that could
in theory also be passed on the command line is awkward, because it
prevents proper error handling for this specific invalid value.

Kevin