Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3650 bytes --]

On 22.02.19 16:38, Kevin Wolf wrote:
> Am 22.02.2019 um 15:12 hat Max Reitz geschrieben:
>> On 19.02.19 09:51, Kevin Wolf wrote:
>>> Am 19.02.2019 um 00:57 hat Max Reitz geschrieben:
>>>> On 31.01.19 18:55, Kevin Wolf wrote:
>>>>> This adds a .bdrv_open option to specify the external data file node.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>> ---
>>>>>  qapi/block-core.json |  3 ++-
>>>>>  block/qcow2.h        |  4 +++-
>>>>>  block/qcow2.c        | 25 +++++++++++++++++++++++--
>>>>>  3 files changed, 28 insertions(+), 4 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>>> index c161970882..e2114900b4 100644
>>>>> --- a/block/qcow2.h
>>>>> +++ b/block/qcow2.h
>>>>
>>>> [...]
>>>>
>>>>> @@ -205,7 +206,8 @@ enum {
>>>>>      QCOW2_INCOMPAT_DATA_FILE        = 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
>>>>>  
>>>>>      QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
>>>>> -                                 | QCOW2_INCOMPAT_CORRUPT,
>>>>> +                                 | QCOW2_INCOMPAT_CORRUPT
>>>>> +                                 | QCOW2_INCOMPAT_DATA_FILE,
>>>>
>>>> This hunk seems to belong somewhere else.
>>>
>>> Isn't this the first patch that actually allows opening images that have
>>> QCOW2_INCOMPAT_DATA_FILE set in their header?
>>
>> Oh, sorry.  I thought the mask was the mask of all incompatible
>> features, but it's actually the mask of supported incompatible features.
>>  Yep, then it's correct here.
>>
>>>>>  };
>>>>>  
>>>>>  /* Compatible feature bits */
>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>> index ac9934b3ed..376232d3f0 100644
>>>>> --- a/block/qcow2.c
>>>>> +++ b/block/qcow2.c
>>>>> @@ -1441,8 +1441,22 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>>>          goto fail;
>>>>>      }
>>>>>  
>>>>> -    /* TODO Open external data file */
>>>>> -    s->data_file = bs->file;
>>>>> +    /* Open external data file */
>>>>> +    if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>>>>> +        s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>>>>> +                                       &child_file, false, errp);
>>>>> +        if (!s->data_file) {
>>>>> +            ret = -EINVAL;
>>>>> +            goto fail;
>>>>> +        }
>>>>> +    } else if (qdict_get(options, QCOW2_OPT_DATA_FILE)) {
>>>>
>>>> I get the idea, but this isn't crumpled so this key may not exist (but
>>>> data-file.driver and data-file.filename may).  Of course the fact that
>>>> these options remain unused will be caught by the block layer, but that
>>>> makes the error message below a bit less useful.
>>>
>>> Hmm, good point... So you'd just leave out the check and always let the
>>> block layer complain (with a less than helpful message)? Or are you
>>> suggesting I should try and catch all cases somehow, even if that makes
>>> things quite a bit more complicated?
>>
>> I don't mind either way.  Nice error messages are nice as long as
>> they're not too much trouble.  It's just that this current state feels a
>> bit half-baked.
> 
> I think catching everything is just not worth the effort. So I
> considered dropping the check. But then I thought that this half baked
> check will make sure that we'll get full coverage once qcow2 is
> converted to a QAPI-based open interface instead of forgetting to bring
> the check back. So I'm leaning towards just leaving it as it is now. If
> you like, I can add a comment that we know this is half baked.

"Once"... :-)

Seems reasonable to me.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]