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/qapi/block-core.json b/qapi/block-core.json
index 7f6b4b3ddd..fc46396079 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2928,7 +2928,8 @@
'*l2-cache-entry-size': 'int',
'*refcount-cache-size': 'int',
'*cache-clean-interval': 'int',
- '*encrypt': 'BlockdevQcow2Encryption' } }
+ '*encrypt': 'BlockdevQcow2Encryption',
+ '*data-file': 'BlockdevRef' } }
##
# @SshHostKeyCheckMode:
diff --git a/block/qcow2.h b/block/qcow2.h
index c161970882..e2114900b4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -91,6 +91,7 @@
#define DEFAULT_CLUSTER_SIZE 65536
+#define QCOW2_OPT_DATA_FILE "data-file"
#define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
#define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
#define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
@@ -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,
};
/* 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)) {
+ error_setg(errp, "'data-file' can only be set for images with an "
+ "external data file");
+ ret = -EINVAL;
+ goto fail;
+ } else {
+ s->data_file = bs->file;
+ }
/* qcow2_read_extension may have set up the crypto context
* if the crypt method needs a header region, some methods
@@ -1613,6 +1627,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
return ret;
fail:
+ if (has_data_file(bs)) {
+ bdrv_unref_child(bs, s->data_file);
+ }
g_free(s->unknown_header_fields);
cleanup_unknown_header_ext(bs);
qcow2_free_snapshots(bs);
@@ -2232,6 +2249,10 @@ static void qcow2_close(BlockDriverState *bs)
g_free(s->image_backing_file);
g_free(s->image_backing_format);
+ if (has_data_file(bs)) {
+ bdrv_unref_child(bs, s->data_file);
+ }
+
qcow2_refcount_close(bs);
qcow2_free_snapshots(bs);
}
--
2.20.1
On 1/31/19 11:55 AM, 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/qapi/block-core.json b/qapi/block-core.json > index 7f6b4b3ddd..fc46396079 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2928,7 +2928,8 @@ > '*l2-cache-entry-size': 'int', > '*refcount-cache-size': 'int', > '*cache-clean-interval': 'int', > - '*encrypt': 'BlockdevQcow2Encryption' } } > + '*encrypt': 'BlockdevQcow2Encryption', > + '*data-file': 'BlockdevRef' } } Missing docs and a '(since 4.0)' tag. Then again, it's still RFC, so that's understandable for this draft. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Am 31.01.2019 um 19:44 hat Eric Blake geschrieben: > On 1/31/19 11:55 AM, 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/qapi/block-core.json b/qapi/block-core.json > > index 7f6b4b3ddd..fc46396079 100644 > > --- a/qapi/block-core.json > > +++ b/qapi/block-core.json > > @@ -2928,7 +2928,8 @@ > > '*l2-cache-entry-size': 'int', > > '*refcount-cache-size': 'int', > > '*cache-clean-interval': 'int', > > - '*encrypt': 'BlockdevQcow2Encryption' } } > > + '*encrypt': 'BlockdevQcow2Encryption', > > + '*data-file': 'BlockdevRef' } } > > Missing docs and a '(since 4.0)' tag. Then again, it's still RFC, so > that's understandable for this draft. Yes, as mentioned in the cover letter. Kevin
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.
> };
>
> /* 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.
Max
> + error_setg(errp, "'data-file' can only be set for images with an "
> + "external data file");
> + ret = -EINVAL;
> + goto fail;
> + } else {
> + s->data_file = bs->file;
> + }
>
> /* qcow2_read_extension may have set up the crypto context
> * if the crypt method needs a header region, some methods
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
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.
Max
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.
Kevin
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
© 2016 - 2026 Red Hat, Inc.