Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
Am 22.02.2019 um 17:13 hat Max Reitz geschrieben:
> On 22.02.19 16:57, Kevin Wolf wrote:
> > Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
> >> On 19.02.19 10:17, Kevin Wolf wrote:
> >>> Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
> On 31.01.19 18:55, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf
> > ---
> > qapi/block-core.json | 1 +
> > block/qcow2.c| 6 +-
> > 2 files changed, 6 insertions(+), 1 deletion(-)
>
> [...]
>
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 4959bf16a4..e3427f9fcd 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1459,7 +1459,9 @@ static int coroutine_fn
> > qcow2_do_open(BlockDriverState *bs, QDict *options,
> > if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> > s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> > _file, false, _err);
> > -if (!s->data_file) {
> > +if (s->data_file) {
> > +s->image_data_file = g_strdup(s->data_file->bs->filename);
> > +} else {
> > if (s->image_data_file) {
> > error_free(local_err);
> > local_err = NULL;
>
> Ah, this is what I looked for in the last patch. :-)
>
> (i.e. it should be in the last patch, not here)
> >>>
> >>> [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
> >>>
> >>> This is the last patch. :-P
> >>
> >> Sorry, I meant the previous one.
> >>
> I think as it is it is just wrong, though. If I pass enough options at
> runtime, this will overwrite the image header:
>
> $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
> $ ./qemu-img create -f raw bar.raw 64M
> $ ./qemu-img info foo.qcow2
> [...]
> data file: foo.raw
> [...]
> $ ./qemu-io --image-opts \
> file.filename=foo.qcow2,data-file.driver=file,\
> data-file.filename=bar.raw,lazy-refcounts=on \
> -c 'write 0 64k'
> # (The lazy-refcounts is so the image header is updated)
> $ ./qemu-img info foo.qcow2
> [...]
> data file: bar.raw
> [...]
>
> The right thing would probably to check whether the header extension
> exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
> is NULL), s->image_data_file gets set; because there are no valid images
> with the external data file flag set where there is no such header
> extension. So we must be in the process of creating the image right now.
>
> But even then, I don't quite like setting it here and not creating the
> header extension as part of qcow2_co_create(). I can see why you've
> done it this way, but creating a "bad" image on purpose (one with the
> external data file bit set, but no such header extension present yet) in
> order to detect and rectify this case when it is first opened (and the
> opening code assuming that any such broken image must be one that is
> opened the first time) is a bit weird.
> >>>
> >>> It's not really a bad image, just one that's a bit cumbersome to use
> >>> because you need to specify the 'data-file' option manually.
> >>
> >> Of course it's bad because it doesn't adhere to the specification (which
> >> you could amend, of course, since you add it with this series). The
> >> spec says "If this bit is set, an external data file name header
> >> extension must be present as well." Which it isn't until the image is
> >> opened with the data-file option.
> >
> > Hm, I wonder whether that's a good requirement to make or whether we
> > should indeed change the spec. It wouldn't be so bad to have images that
> > require the data-file runtime option.
> >
> > I guess we could lift the restriction later if we want to make use of
> > it. But the QEMU code is already written in a way that this works, so
> > maybe just allow it.
>
> OK for me.
>
> I suppose doing it right (if you agree with the paragraph before the
> last one) and adding a comment would make it less weird
> ("s->image_data_file must be non-NULL for any valid image, so this image
> must be one we are creating right now" or something like that).
>
> But still, the issue you point out in your cover letter remains; which
> is that the node's filename and the filename given by the user may be
> two different things.
> >>>
> >>> I think what I was planning to do was leaving the path from the image
> >>> header in s->image_data_file even when a runtime option overrides it.
> >>> After all, ImageInfo is about the image, not about the runtime state.
> >>
> >> I'm not talking about ImageInfo here, though, I'm talking about the
> >> image creation process. The hunk I've quoted should be in the previous
> >> patch, not in this one.
> >>
> >> Which doesn't make
Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
On 22.02.19 16:57, Kevin Wolf wrote:
> Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
>> On 19.02.19 10:17, Kevin Wolf wrote:
>>> Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
On 31.01.19 18:55, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf
> ---
> qapi/block-core.json | 1 +
> block/qcow2.c| 6 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
[...]
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4959bf16a4..e3427f9fcd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1459,7 +1459,9 @@ static int coroutine_fn
> qcow2_do_open(BlockDriverState *bs, QDict *options,
> if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> _file, false, _err);
> -if (!s->data_file) {
> +if (s->data_file) {
> +s->image_data_file = g_strdup(s->data_file->bs->filename);
> +} else {
> if (s->image_data_file) {
> error_free(local_err);
> local_err = NULL;
Ah, this is what I looked for in the last patch. :-)
(i.e. it should be in the last patch, not here)
>>>
>>> [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
>>>
>>> This is the last patch. :-P
>>
>> Sorry, I meant the previous one.
>>
I think as it is it is just wrong, though. If I pass enough options at
runtime, this will overwrite the image header:
$ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
$ ./qemu-img create -f raw bar.raw 64M
$ ./qemu-img info foo.qcow2
[...]
data file: foo.raw
[...]
$ ./qemu-io --image-opts \
file.filename=foo.qcow2,data-file.driver=file,\
data-file.filename=bar.raw,lazy-refcounts=on \
-c 'write 0 64k'
# (The lazy-refcounts is so the image header is updated)
$ ./qemu-img info foo.qcow2
[...]
data file: bar.raw
[...]
The right thing would probably to check whether the header extension
exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
is NULL), s->image_data_file gets set; because there are no valid images
with the external data file flag set where there is no such header
extension. So we must be in the process of creating the image right now.
But even then, I don't quite like setting it here and not creating the
header extension as part of qcow2_co_create(). I can see why you've
done it this way, but creating a "bad" image on purpose (one with the
external data file bit set, but no such header extension present yet) in
order to detect and rectify this case when it is first opened (and the
opening code assuming that any such broken image must be one that is
opened the first time) is a bit weird.
>>>
>>> It's not really a bad image, just one that's a bit cumbersome to use
>>> because you need to specify the 'data-file' option manually.
>>
>> Of course it's bad because it doesn't adhere to the specification (which
>> you could amend, of course, since you add it with this series). The
>> spec says "If this bit is set, an external data file name header
>> extension must be present as well." Which it isn't until the image is
>> opened with the data-file option.
>
> Hm, I wonder whether that's a good requirement to make or whether we
> should indeed change the spec. It wouldn't be so bad to have images that
> require the data-file runtime option.
>
> I guess we could lift the restriction later if we want to make use of
> it. But the QEMU code is already written in a way that this works, so
> maybe just allow it.
OK for me.
I suppose doing it right (if you agree with the paragraph before the
last one) and adding a comment would make it less weird
("s->image_data_file must be non-NULL for any valid image, so this image
must be one we are creating right now" or something like that).
But still, the issue you point out in your cover letter remains; which
is that the node's filename and the filename given by the user may be
two different things.
>>>
>>> I think what I was planning to do was leaving the path from the image
>>> header in s->image_data_file even when a runtime option overrides it.
>>> After all, ImageInfo is about the image, not about the runtime state.
>>
>> I'm not talking about ImageInfo here, though, I'm talking about the
>> image creation process. The hunk I've quoted should be in the previous
>> patch, not in this one.
>>
>> Which doesn't make wrong what you're saying, though, the ImageInfo
>> should print what's in the header.
>>
>>> Image creation would just manually set s->image_data_file before
>>> updating the header.
>>
>> It should, but currently it does that rather indirectly (by setting the
>>
Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
Am 22.02.2019 um 14:51 hat Max Reitz geschrieben:
> On 19.02.19 10:17, Kevin Wolf wrote:
> > Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
> >> On 31.01.19 18:55, Kevin Wolf wrote:
> >>> Signed-off-by: Kevin Wolf
> >>> ---
> >>> qapi/block-core.json | 1 +
> >>> block/qcow2.c| 6 +-
> >>> 2 files changed, 6 insertions(+), 1 deletion(-)
> >>
> >> [...]
> >>
> >>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>> index 4959bf16a4..e3427f9fcd 100644
> >>> --- a/block/qcow2.c
> >>> +++ b/block/qcow2.c
> >>> @@ -1459,7 +1459,9 @@ static int coroutine_fn
> >>> qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>> if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> >>> s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> >>> _file, false, _err);
> >>> -if (!s->data_file) {
> >>> +if (s->data_file) {
> >>> +s->image_data_file = g_strdup(s->data_file->bs->filename);
> >>> +} else {
> >>> if (s->image_data_file) {
> >>> error_free(local_err);
> >>> local_err = NULL;
> >>
> >> Ah, this is what I looked for in the last patch. :-)
> >>
> >> (i.e. it should be in the last patch, not here)
> >
> > [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
> >
> > This is the last patch. :-P
>
> Sorry, I meant the previous one.
>
> >> I think as it is it is just wrong, though. If I pass enough options at
> >> runtime, this will overwrite the image header:
> >>
> >> $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
> >> $ ./qemu-img create -f raw bar.raw 64M
> >> $ ./qemu-img info foo.qcow2
> >> [...]
> >> data file: foo.raw
> >> [...]
> >> $ ./qemu-io --image-opts \
> >> file.filename=foo.qcow2,data-file.driver=file,\
> >> data-file.filename=bar.raw,lazy-refcounts=on \
> >> -c 'write 0 64k'
> >> # (The lazy-refcounts is so the image header is updated)
> >> $ ./qemu-img info foo.qcow2
> >> [...]
> >> data file: bar.raw
> >> [...]
> >>
> >> The right thing would probably to check whether the header extension
> >> exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
> >> is NULL), s->image_data_file gets set; because there are no valid images
> >> with the external data file flag set where there is no such header
> >> extension. So we must be in the process of creating the image right now.
> >>
> >> But even then, I don't quite like setting it here and not creating the
> >> header extension as part of qcow2_co_create(). I can see why you've
> >> done it this way, but creating a "bad" image on purpose (one with the
> >> external data file bit set, but no such header extension present yet) in
> >> order to detect and rectify this case when it is first opened (and the
> >> opening code assuming that any such broken image must be one that is
> >> opened the first time) is a bit weird.
> >
> > It's not really a bad image, just one that's a bit cumbersome to use
> > because you need to specify the 'data-file' option manually.
>
> Of course it's bad because it doesn't adhere to the specification (which
> you could amend, of course, since you add it with this series). The
> spec says "If this bit is set, an external data file name header
> extension must be present as well." Which it isn't until the image is
> opened with the data-file option.
Hm, I wonder whether that's a good requirement to make or whether we
should indeed change the spec. It wouldn't be so bad to have images that
require the data-file runtime option.
I guess we could lift the restriction later if we want to make use of
it. But the QEMU code is already written in a way that this works, so
maybe just allow it.
> >> I suppose doing it right (if you agree with the paragraph before the
> >> last one) and adding a comment would make it less weird
> >> ("s->image_data_file must be non-NULL for any valid image, so this image
> >> must be one we are creating right now" or something like that).
> >>
> >> But still, the issue you point out in your cover letter remains; which
> >> is that the node's filename and the filename given by the user may be
> >> two different things.
> >
> > I think what I was planning to do was leaving the path from the image
> > header in s->image_data_file even when a runtime option overrides it.
> > After all, ImageInfo is about the image, not about the runtime state.
>
> I'm not talking about ImageInfo here, though, I'm talking about the
> image creation process. The hunk I've quoted should be in the previous
> patch, not in this one.
>
> Which doesn't make wrong what you're saying, though, the ImageInfo
> should print what's in the header.
>
> > Image creation would just manually set s->image_data_file before
> > updating the header.
>
> It should, but currently it does that rather indirectly (by setting the
> data-file option which then makes qcow2_do_open() copy it into
>
Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
On 19.02.19 10:17, Kevin Wolf wrote:
> Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf
>>> ---
>>> qapi/block-core.json | 1 +
>>> block/qcow2.c| 6 +-
>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 4959bf16a4..e3427f9fcd 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1459,7 +1459,9 @@ static int coroutine_fn
>>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>> if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
>>> s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
>>> _file, false, _err);
>>> -if (!s->data_file) {
>>> +if (s->data_file) {
>>> +s->image_data_file = g_strdup(s->data_file->bs->filename);
>>> +} else {
>>> if (s->image_data_file) {
>>> error_free(local_err);
>>> local_err = NULL;
>>
>> Ah, this is what I looked for in the last patch. :-)
>>
>> (i.e. it should be in the last patch, not here)
>
> [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
>
> This is the last patch. :-P
Sorry, I meant the previous one.
>> I think as it is it is just wrong, though. If I pass enough options at
>> runtime, this will overwrite the image header:
>>
>> $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
>> $ ./qemu-img create -f raw bar.raw 64M
>> $ ./qemu-img info foo.qcow2
>> [...]
>> data file: foo.raw
>> [...]
>> $ ./qemu-io --image-opts \
>> file.filename=foo.qcow2,data-file.driver=file,\
>> data-file.filename=bar.raw,lazy-refcounts=on \
>> -c 'write 0 64k'
>> # (The lazy-refcounts is so the image header is updated)
>> $ ./qemu-img info foo.qcow2
>> [...]
>> data file: bar.raw
>> [...]
>>
>> The right thing would probably to check whether the header extension
>> exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
>> is NULL), s->image_data_file gets set; because there are no valid images
>> with the external data file flag set where there is no such header
>> extension. So we must be in the process of creating the image right now.
>>
>> But even then, I don't quite like setting it here and not creating the
>> header extension as part of qcow2_co_create(). I can see why you've
>> done it this way, but creating a "bad" image on purpose (one with the
>> external data file bit set, but no such header extension present yet) in
>> order to detect and rectify this case when it is first opened (and the
>> opening code assuming that any such broken image must be one that is
>> opened the first time) is a bit weird.
>
> It's not really a bad image, just one that's a bit cumbersome to use
> because you need to specify the 'data-file' option manually.
Of course it's bad because it doesn't adhere to the specification (which
you could amend, of course, since you add it with this series). The
spec says "If this bit is set, an external data file name header
extension must be present as well." Which it isn't until the image is
opened with the data-file option.
>> I suppose doing it right (if you agree with the paragraph before the
>> last one) and adding a comment would make it less weird
>> ("s->image_data_file must be non-NULL for any valid image, so this image
>> must be one we are creating right now" or something like that).
>>
>> But still, the issue you point out in your cover letter remains; which
>> is that the node's filename and the filename given by the user may be
>> two different things.
>
> I think what I was planning to do was leaving the path from the image
> header in s->image_data_file even when a runtime option overrides it.
> After all, ImageInfo is about the image, not about the runtime state.
I'm not talking about ImageInfo here, though, I'm talking about the
image creation process. The hunk I've quoted should be in the previous
patch, not in this one.
Which doesn't make wrong what you're saying, though, the ImageInfo
should print what's in the header.
> Image creation would just manually set s->image_data_file before
> updating the header.
It should, but currently it does that rather indirectly (by setting the
data-file option which then makes qcow2_do_open() copy it into
s->image_data_file).
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
On 2/19/19 3:17 AM, Kevin Wolf wrote: >> >> Ah, this is what I looked for in the last patch. :-) >> >> (i.e. it should be in the last patch, not here) > > [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2 > > This is the last patch. :-P "last"=="previous" (10/11), not "last"=="final" (11/11). Isn't English great? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
Am 19.02.2019 um 01:47 hat Max Reitz geschrieben:
> On 31.01.19 18:55, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf
> > ---
> > qapi/block-core.json | 1 +
> > block/qcow2.c| 6 +-
> > 2 files changed, 6 insertions(+), 1 deletion(-)
>
> [...]
>
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 4959bf16a4..e3427f9fcd 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1459,7 +1459,9 @@ static int coroutine_fn
> > qcow2_do_open(BlockDriverState *bs, QDict *options,
> > if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> > s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> > _file, false, _err);
> > -if (!s->data_file) {
> > +if (s->data_file) {
> > +s->image_data_file = g_strdup(s->data_file->bs->filename);
> > +} else {
> > if (s->image_data_file) {
> > error_free(local_err);
> > local_err = NULL;
>
> Ah, this is what I looked for in the last patch. :-)
>
> (i.e. it should be in the last patch, not here)
[RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
This is the last patch. :-P
> I think as it is it is just wrong, though. If I pass enough options at
> runtime, this will overwrite the image header:
>
> $ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
> $ ./qemu-img create -f raw bar.raw 64M
> $ ./qemu-img info foo.qcow2
> [...]
> data file: foo.raw
> [...]
> $ ./qemu-io --image-opts \
> file.filename=foo.qcow2,data-file.driver=file,\
> data-file.filename=bar.raw,lazy-refcounts=on \
> -c 'write 0 64k'
> # (The lazy-refcounts is so the image header is updated)
> $ ./qemu-img info foo.qcow2
> [...]
> data file: bar.raw
> [...]
>
> The right thing would probably to check whether the header extension
> exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
> is NULL), s->image_data_file gets set; because there are no valid images
> with the external data file flag set where there is no such header
> extension. So we must be in the process of creating the image right now.
>
> But even then, I don't quite like setting it here and not creating the
> header extension as part of qcow2_co_create(). I can see why you've
> done it this way, but creating a "bad" image on purpose (one with the
> external data file bit set, but no such header extension present yet) in
> order to detect and rectify this case when it is first opened (and the
> opening code assuming that any such broken image must be one that is
> opened the first time) is a bit weird.
It's not really a bad image, just one that's a bit cumbersome to use
because you need to specify the 'data-file' option manually.
> I suppose doing it right (if you agree with the paragraph before the
> last one) and adding a comment would make it less weird
> ("s->image_data_file must be non-NULL for any valid image, so this image
> must be one we are creating right now" or something like that).
>
> But still, the issue you point out in your cover letter remains; which
> is that the node's filename and the filename given by the user may be
> two different things.
I think what I was planning to do was leaving the path from the image
header in s->image_data_file even when a runtime option overrides it.
After all, ImageInfo is about the image, not about the runtime state.
Image creation would just manually set s->image_data_file before
updating the header.
Kevin
signature.asc
Description: PGP signature
Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
On 31.01.19 18:55, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf
> ---
> qapi/block-core.json | 1 +
> block/qcow2.c| 6 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
[...]
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4959bf16a4..e3427f9fcd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1459,7 +1459,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
> *bs, QDict *options,
> if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
> s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
> _file, false, _err);
> -if (!s->data_file) {
> +if (s->data_file) {
> +s->image_data_file = g_strdup(s->data_file->bs->filename);
> +} else {
> if (s->image_data_file) {
> error_free(local_err);
> local_err = NULL;
Ah, this is what I looked for in the last patch. :-)
(i.e. it should be in the last patch, not here)
I think as it is it is just wrong, though. If I pass enough options at
runtime, this will overwrite the image header:
$ ./qemu-img create -f qcow2 -o data_file=foo.raw foo.qcow2 64M
$ ./qemu-img create -f raw bar.raw 64M
$ ./qemu-img info foo.qcow2
[...]
data file: foo.raw
[...]
$ ./qemu-io --image-opts \
file.filename=foo.qcow2,data-file.driver=file,\
data-file.filename=bar.raw,lazy-refcounts=on \
-c 'write 0 64k'
# (The lazy-refcounts is so the image header is updated)
$ ./qemu-img info foo.qcow2
[...]
data file: bar.raw
[...]
The right thing would probably to check whether the header extension
exists (i.e. if s->image_data_file is non-NULL) and if it does not (it
is NULL), s->image_data_file gets set; because there are no valid images
with the external data file flag set where there is no such header
extension. So we must be in the process of creating the image right now.
But even then, I don't quite like setting it here and not creating the
header extension as part of qcow2_co_create(). I can see why you've
done it this way, but creating a "bad" image on purpose (one with the
external data file bit set, but no such header extension present yet) in
order to detect and rectify this case when it is first opened (and the
opening code assuming that any such broken image must be one that is
opened the first time) is a bit weird.
I suppose doing it right (if you agree with the paragraph before the
last one) and adding a comment would make it less weird
("s->image_data_file must be non-NULL for any valid image, so this image
must be one we are creating right now" or something like that).
But still, the issue you point out in your cover letter remains; which
is that the node's filename and the filename given by the user may be
two different things.
Max
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
Signed-off-by: Kevin Wolf
---
qapi/block-core.json | 1 +
block/qcow2.c| 6 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 060df28797..0eb0637b64 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -74,6 +74,7 @@
{ 'struct': 'ImageInfoSpecificQCow2',
'data': {
'compat': 'str',
+ '*data-file': 'str',
'*lazy-refcounts': 'bool',
'*corrupt': 'bool',
'refcount-bits': 'int',
diff --git a/block/qcow2.c b/block/qcow2.c
index 4959bf16a4..e3427f9fcd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1459,7 +1459,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState
*bs, QDict *options,
if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
_file, false, _err);
-if (!s->data_file) {
+if (s->data_file) {
+s->image_data_file = g_strdup(s->data_file->bs->filename);
+} else {
if (s->image_data_file) {
error_free(local_err);
local_err = NULL;
@@ -4533,6 +4535,8 @@ static ImageInfoSpecific
*qcow2_get_specific_info(BlockDriverState *bs)
QCOW2_INCOMPAT_CORRUPT,
.has_corrupt= true,
.refcount_bits = s->refcount_bits,
+.has_data_file = !!s->image_data_file,
+.data_file = g_strdup(s->image_data_file),
};
} else {
/* if this assertion fails, this probably means a new version was
--
2.20.1
