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

[Kevin Wolf <kwolf@redhat.com>]

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.

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

> The TODO rather also applies to encryption; however, since the worst
> it does there is generate an error message, it isn't as bad as here
> (were the code might actually change the image if the flag is not
> given).

Agreed, it "only" requires that the user specify the encryption flag
explicitly when changing options of an encrypted image. No disaster
happens, it's just unfriendly.

Kevin