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
