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

[Kevin Wolf <kwolf@redhat.com>]

Am 02.09.2013 um 05:43 hat Fam Zheng geschrieben:
> On Fri, 08/30 12:27, Max Reitz wrote:
> > Implement bdrv_amend_options for compat, size, backing_file, backing_fmt
> > and lazy_refcounts.
> > 
> > Downgrading images from compat=1.1 to compat=0.10 is achieved through
> > handling all incompatible flags accordingly, clearing all compatible and
> > autoclear flags and expanding all zero clusters.
> > 
> > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > ---
> >  block/qcow2-cluster.c | 165 ++++++++++++++++++++++++++++++++++++++++++
> >  block/qcow2.c         | 194 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  block/qcow2.h         |   3 +
> >  3 files changed, 361 insertions(+), 1 deletion(-)

> > +static int qcow2_amend_options(BlockDriverState *bs,
> > +                               QEMUOptionParameter *options)
> > +{
> > +    BDRVQcowState *s = bs->opaque;
> > +    int old_version = s->qcow_version, new_version = old_version;
> > +    uint64_t new_size = 0;
> > +    const char *backing_file = NULL, *backing_format = NULL;
> > +    bool lazy_refcounts = s->use_lazy_refcounts;
> > +    int ret;
> > +    int i;
> > +
> > +    for (i = 0; options[i].name; i++)
> > +    {
> > +        if (!strcmp(options[i].name, "compat")) {
> > +            if (!options[i].value.s) {
> > +                /* preserve default */
> > +            } else if (!strcmp(options[i].value.s, "0.10")) {
> > +                new_version = 2;
> > +            } else if (!strcmp(options[i].value.s, "1.1")) {
> > +                new_version = 3;
> > +            } else {
> > +                fprintf(stderr, "Unknown compatibility level %s.\n",
> > +                        options[i].value.s);
> > +                return -EINVAL;
> > +            }
> > +        } else if (!strcmp(options[i].name, "preallocation")) {
> > +            if (options[i].assigned) {
> 
> For encryption flag and cluster_size, you checked the original value and only
> error out on actual change, should check the original preallocation value here
> as well?
> 
> > +                fprintf(stderr, "Cannot change preallocation mode.\n");
> > +                return -ENOTSUP;
> > +            }
> > +        } else if (!strcmp(options[i].name, "size")) {
> > +            new_size = options[i].value.n;
> > +        } else if (!strcmp(options[i].name, "backing_file")) {
> > +            backing_file = options[i].value.s;
> > +        } else if (!strcmp(options[i].name, "backing_fmt")) {
> > +            backing_format = options[i].value.s;
> > +        } else if (!strcmp(options[i].name, "encryption")) {
> > +            if (options[i].assigned &&
> > +                (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].assigned && (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")) {
> > +            if (options[i].assigned) {
> > +                lazy_refcounts = options[i].value.n;
> > +            }
> > +        } else {
> > +            /* if this assertion fails, this probably means a new option was
> > +             * added without having it covered here */
> > +            assert(false);
> 
> A unknown option reported as -ENOTSUP with a proper message is good enough,
> it's not that critical for an assert.

assert() has nothing to do with being critical.

This is not code for handling a user error, but for catching programming
errors. If the else branch is reached, this is a bug, and you want it to
be detected as quickly as possible (and without having to debug a lot in
order to find out where a bogus -ENOTSUP comes from)

Kevin