[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [RFC PATCH 08/11] qcow2: Add basic data-file infrastruc
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure |
Date: |
Fri, 22 Feb 2019 16:38:53 +0100 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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 <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.
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.
Kevin
signature.asc
Description: PGP signature