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 <mre...@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
