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

[Max Reitz]

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

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


Max