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

[Max Reitz <mreitz@redhat.com>]

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