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

[Kevin Wolf <kwolf@redhat.com>]

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

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?

> >  };
> >  
> >  /* 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?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]