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 <kw...@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. Kevin
signature.asc
Description: PGP signature