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

[Max Reitz]

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 <address@hidden>
>>> ---
>>>  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.

Max

signature.asc
Description: OpenPGP digital signature