* [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 18:43 ` Eric Blake
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 02/11] qcow2: Basic definitions " Kevin Wolf
` (11 subsequent siblings)
12 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
This adds external data file to the qcow2 spec as a new incompatible
feature.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
docs/interop/qcow2.txt | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index fb5cb47245..90af0d02e5 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -97,7 +97,17 @@ in the description of a field.
be written to (unless for regaining
consistency).
- Bits 2-63: Reserved (set to 0)
+ Bit 2: External data file. If this bit is set, an
+ external data file name header extension must
+ be present as well. Guest clusters are then
+ stored in the external data file. For such
+ images, clusters in the external data file are
+ not refcounted. The offset field in the
+ Standard Cluster Descriptor must match the
+ guest offset and neither compressed clusters
+ nor internal snapshots are supported.
+
+ Bits 3-63: Reserved (set to 0)
80 - 87: compatible_features
Bitmask of compatible features. An implementation can
@@ -148,6 +158,7 @@ be stored. Each extension has a structure like the following:
0x6803f857 - Feature name table
0x23852875 - Bitmaps extension
0x0537be77 - Full disk encryption header pointer
+ 0x44415441 - External data file name
other - Unknown header extension, can be safely
ignored
@@ -450,8 +461,10 @@ Standard Cluster Descriptor:
1 - 8: Reserved (set to 0)
9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a
- cluster boundary. If the offset is 0, the cluster is
- unallocated.
+ cluster boundary. If the offset is 0 and bit 63 is clear,
+ the cluster is unallocated. The offset may only be 0 with
+ bit 63 set (indicating a host cluster offset of 0) when an
+ external data file is used.
56 - 61: Reserved (set to 0)
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external " Kevin Wolf
@ 2019-01-31 18:43 ` Eric Blake
2019-01-31 21:44 ` [Qemu-devel] [Qemu-block] " Nir Soffer
2019-02-01 10:21 ` [Qemu-devel] " Kevin Wolf
0 siblings, 2 replies; 46+ messages in thread
From: Eric Blake @ 2019-01-31 18:43 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3099 bytes --]
On 1/31/19 11:55 AM, Kevin Wolf wrote:
> This adds external data file to the qcow2 spec as a new incompatible
> feature.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> docs/interop/qcow2.txt | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> @@ -148,6 +158,7 @@ be stored. Each extension has a structure like the following:
> 0x6803f857 - Feature name table
> 0x23852875 - Bitmaps extension
> 0x0537be77 - Full disk encryption header pointer
> + 0x44415441 - External data file name
> other - Unknown header extension, can be safely
> ignored
Missing a section describing the new header.
>
> @@ -450,8 +461,10 @@ Standard Cluster Descriptor:
> 1 - 8: Reserved (set to 0)
>
> 9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a
> - cluster boundary. If the offset is 0, the cluster is
> - unallocated.
> + cluster boundary. If the offset is 0 and bit 63 is clear,
> + the cluster is unallocated. The offset may only be 0 with
> + bit 63 set (indicating a host cluster offset of 0) when an
> + external data file is used.
Does that mean that the value 0x00000000 is invalid for external data
files, and that 0x00000001 is special-cased to mean read the contents of
the external file (and NOT that the cluster reads as all zeroes)? Is
bit 0 allowed to be set for any other clusters when there is an external
data file? And if so, are we requiring that it only be set when the
external file is known to read as zero, or can we run into the situation
where qcow2 says the cluster reads as 0 but the host file contains
garbage? Should the external file header contain a flag that states
whether writes to the image should wipe vs. leave unchanged a cluster in
the external file when the qcow2 metadata prefers to grab that cluster's
contents as all-0s or by reading from the backing file? There are
security vs. speed implications - security insists on wiping the host
file to NOT leave stale data, but that slows things down compared to
just leaving garbage if the qcow2 metadata can effectively ignore those
parts of the external file - hence a knob may be worth exposing?
Should we preserve the meaning of bit 0 SOLELY for its 'reads-as-zeroes'
meaning, and instead make bit 1 (currently reserved) as the special
marker that MUST be set for clusters read from the external file,
keeping the two ideas orthogonal?
Worth a mention on the reftable section that when an external file is
used, metadata clusters (in the qcow2 file) are still refcounted (and
thus, offsets in the refcount table point to the qcow2 file, not the
external file)?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-01-31 18:43 ` Eric Blake
@ 2019-01-31 21:44 ` Nir Soffer
2019-02-01 10:29 ` Kevin Wolf
2019-02-01 10:21 ` [Qemu-devel] " Kevin Wolf
1 sibling, 1 reply; 46+ messages in thread
From: Nir Soffer @ 2019-01-31 21:44 UTC (permalink / raw)
To: Eric Blake; +Cc: Kevin Wolf, qemu-block, QEMU Developers, Max Reitz, Adam Litke
On Thu, Jan 31, 2019 at 8:43 PM Eric Blake <eblake@redhat.com> wrote:
...
> > @@ -450,8 +461,10 @@ Standard Cluster Descriptor:
>
> 1 - 8: Reserved (set to 0)
> >
> > 9 - 55: Bits 9-55 of host cluster offset. Must be aligned
> to a
> > - cluster boundary. If the offset is 0, the cluster is
> > - unallocated.
> > + cluster boundary. If the offset is 0 and bit 63 is
> clear,
> > + the cluster is unallocated. The offset may only be
> 0 with
> > + bit 63 set (indicating a host cluster offset of 0)
> when an
> > + external data file is used.
>
> Does that mean that the value 0x00000000 is invalid for external data
> files, and that 0x00000001 is special-cased to mean read the contents of
> the external file (and NOT that the cluster reads as all zeroes)? Is
> bit 0 allowed to be set for any other clusters when there is an external
> data file? And if so, are we requiring that it only be set when the
> external file is known to read as zero, or can we run into the situation
> where qcow2 says the cluster reads as 0 but the host file contains
> garbage? Should the external file header contain a flag that states
> whether writes to the image should wipe vs. leave unchanged a cluster in
> the external file when the qcow2 metadata prefers to grab that cluster's
> contents as all-0s or by reading from the backing file?
I hope we are not going to mix qcow2 layer with data file and backing
file in the same chain.
For oVirt we would never want to have a backing chain when using qcow2
metadata layer. I think this is the same use case for KubeVirt.
> There are
> security vs. speed implications - security insists on wiping the host
> file to NOT leave stale data, but that slows things down compared to
> just leaving garbage if the qcow2 metadata can effectively ignore those
> parts of the external file - hence a knob may be worth exposing?
>
Since qcow2 is just used to managed metadata about a raw file, I don't
think it should do any optimizations like this.
What if we implement this differently - a qcow2 layer that keeps only
metadata for a backing file?
All reads will go directly to the backing file, since there is no data in
the
qcow2 file. All writes will go directly to the backing file, since no data
should
be in the qcow2 file. But before writing, update the qcow2 metadata to
reference
the cluster that will be written to the backing file.
Nir
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-01-31 21:44 ` [Qemu-devel] [Qemu-block] " Nir Soffer
@ 2019-02-01 10:29 ` Kevin Wolf
0 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-02-01 10:29 UTC (permalink / raw)
To: Nir Soffer; +Cc: Eric Blake, qemu-block, QEMU Developers, Max Reitz, Adam Litke
Am 31.01.2019 um 22:44 hat Nir Soffer geschrieben:
> On Thu, Jan 31, 2019 at 8:43 PM Eric Blake <eblake@redhat.com> wrote:
> ...
>
> > > @@ -450,8 +461,10 @@ Standard Cluster Descriptor:
> >
> > 1 - 8: Reserved (set to 0)
> > >
> > > 9 - 55: Bits 9-55 of host cluster offset. Must be aligned
> > to a
> > > - cluster boundary. If the offset is 0, the cluster is
> > > - unallocated.
> > > + cluster boundary. If the offset is 0 and bit 63 is
> > clear,
> > > + the cluster is unallocated. The offset may only be
> > 0 with
> > > + bit 63 set (indicating a host cluster offset of 0)
> > when an
> > > + external data file is used.
> >
> > Does that mean that the value 0x00000000 is invalid for external data
> > files, and that 0x00000001 is special-cased to mean read the contents of
> > the external file (and NOT that the cluster reads as all zeroes)? Is
> > bit 0 allowed to be set for any other clusters when there is an external
> > data file? And if so, are we requiring that it only be set when the
> > external file is known to read as zero, or can we run into the situation
> > where qcow2 says the cluster reads as 0 but the host file contains
> > garbage? Should the external file header contain a flag that states
> > whether writes to the image should wipe vs. leave unchanged a cluster in
> > the external file when the qcow2 metadata prefers to grab that cluster's
> > contents as all-0s or by reading from the backing file?
>
> I hope we are not going to mix qcow2 layer with data file and backing
> file in the same chain.
>
> For oVirt we would never want to have a backing chain when using qcow2
> metadata layer. I think this is the same use case for KubeVirt.
There is no reason not to allow this setup. In fact, it would be more
work to forbid it than to just let it happen.
Of course, if you use an external data file and have a backing file,
too, you can't use the external data file as a raw image any more (but
you're not supposed to do that anyway, except maybe for copying it
away or if you want to drop all associated metadata).
> > There are security vs. speed implications - security insists on
> > wiping the host file to NOT leave stale data, but that slows things
> > down compared to just leaving garbage if the qcow2 metadata can
> > effectively ignore those parts of the external file - hence a knob
> > may be worth exposing?
>
> Since qcow2 is just used to managed metadata about a raw file, I don't
> think it should do any optimizations like this.
>
> What if we implement this differently - a qcow2 layer that keeps only
> metadata for a backing file?
>
> All reads will go directly to the backing file, since there is no data
> in the qcow2 file. All writes will go directly to the backing file,
> since no data should be in the qcow2 file. But before writing, update
> the qcow2 metadata to reference the cluster that will be written to
> the backing file.
Writable backing files are a design that we considered, but it wouldn't
be as flexible (you couldn't have a real backing file at the same time,
and you probably also would know the cluster allocation status), but it
would be harder to implement.
Kevin
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-01-31 18:43 ` Eric Blake
2019-01-31 21:44 ` [Qemu-devel] [Qemu-block] " Nir Soffer
@ 2019-02-01 10:21 ` Kevin Wolf
2019-02-01 16:17 ` Eric Blake
1 sibling, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-01 10:21 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-block, mreitz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5560 bytes --]
Am 31.01.2019 um 19:43 hat Eric Blake geschrieben:
> On 1/31/19 11:55 AM, Kevin Wolf wrote:
> > This adds external data file to the qcow2 spec as a new incompatible
> > feature.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > docs/interop/qcow2.txt | 19 ++++++++++++++++---
> > 1 file changed, 16 insertions(+), 3 deletions(-)
> >
>
> > @@ -148,6 +158,7 @@ be stored. Each extension has a structure like the following:
> > 0x6803f857 - Feature name table
> > 0x23852875 - Bitmaps extension
> > 0x0537be77 - Full disk encryption header pointer
> > + 0x44415441 - External data file name
> > other - Unknown header extension, can be safely
> > ignored
>
> Missing a section describing the new header.
The header extension that have a separate section are those that contain
structured data. The backing file format name doesn't have one because
it's just a string, and so is the external data file name.
I guess I could add a section to describe the general concept of
external data files, if you think we have information to put there. All
that is really required should be contained in the feature bit
description already.
> > @@ -450,8 +461,10 @@ Standard Cluster Descriptor:
> > 1 - 8: Reserved (set to 0)
> >
> > 9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a
> > - cluster boundary. If the offset is 0, the cluster is
> > - unallocated.
> > + cluster boundary. If the offset is 0 and bit 63 is clear,
> > + the cluster is unallocated. The offset may only be 0 with
> > + bit 63 set (indicating a host cluster offset of 0) when an
> > + external data file is used.
>
> Does that mean that the value 0x00000000 is invalid for external data
> files
Not sure whether you mean the L2 entry as 0x00000000 or the offset field
as 0x00000000 (neither of them are 32 bit like your value).
In any case, 0 is a valid value for an L2 entry, it means an unallocated
cluster. The same offset, but with bit 63 set (0x8000000000000000) is
valid only for external data files and means an allocated cluster at
offset 0.
> and that 0x00000001 is special-cased to mean read the contents of
> the external file (and NOT that the cluster reads as all zeroes)?
No, where do you read that? This is the description of the offset
field, and bit 0 isn't mentioned anywhere. The zero flag works the same
way as it always does.
> Is bit 0 allowed to be set for any other clusters when there is an
> external data file?
Wait, what are "other clusters"? Are you assuming that we have an image
that stores guest data both internally and externally, in some kind of a
mixed setup?
The idea is that the feature bit signals that _all_ guest data is stored
in the external data file. The offset in the L2 table always refers to
the external file then. Only metadata remains in the qcow2 file.
> And if so, are we requiring that it only be set
> when the external file is known to read as zero, or can we run into
> the situation where qcow2 says the cluster reads as 0 but the host
> file contains garbage?
Hm... This is a good point. Currently it behaves just like normal qcow2
files, but this means that the external file can contain stale data and
the zero flag determines the content. This makes it impossible to use
the external data file as a raw image. So I think we need to add a
requirement to write actual zeroes to the external file there.
> Should the external file header contain a flag
> that states whether writes to the image should wipe vs. leave
> unchanged a cluster in the external file when the qcow2 metadata
> prefers to grab that cluster's contents as all-0s or by reading from
> the backing file? There are security vs. speed implications -
> security insists on wiping the host file to NOT leave stale data, but
> that slows things down compared to just leaving garbage if the qcow2
> metadata can effectively ignore those parts of the external file -
> hence a knob may be worth exposing?
If your argument is security, wouldn't the same tradeoff apply to
internally stored data as well? zero_in_l2_slice() only adds the zero
flag, without overwriting the data in bs->file.
But then, for actual security, wouldn't we need to do an explicit write
rather that write_zeroes so that the same problem doesn't reoccur just
one layer down? Or potentially even more specialised operations?
Security is a different goal than just keeping the data file consistent
enough to be readable as a raw image.
> Should we preserve the meaning of bit 0 SOLELY for its 'reads-as-zeroes'
> meaning, and instead make bit 1 (currently reserved) as the special
> marker that MUST be set for clusters read from the external file,
> keeping the two ideas orthogonal?
We don't need bit 1, _all_ clusters are read from the external file.
> Worth a mention on the reftable section that when an external file is
> used, metadata clusters (in the qcow2 file) are still refcounted (and
> thus, offsets in the refcount table point to the qcow2 file, not the
> external file)?
I don't see how you could interpret the spec otherwise, but if you think
it's helpful, I guess being explicit can never hurt...
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-02-01 10:21 ` [Qemu-devel] " Kevin Wolf
@ 2019-02-01 16:17 ` Eric Blake
2019-02-01 16:53 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Eric Blake @ 2019-02-01 16:17 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, mreitz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 8582 bytes --]
On 2/1/19 4:21 AM, Kevin Wolf wrote:
> Am 31.01.2019 um 19:43 hat Eric Blake geschrieben:
>> On 1/31/19 11:55 AM, Kevin Wolf wrote:
>>> This adds external data file to the qcow2 spec as a new incompatible
>>> feature.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> docs/interop/qcow2.txt | 19 ++++++++++++++++---
>>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>
>>> @@ -148,6 +158,7 @@ be stored. Each extension has a structure like the following:
>>> 0x6803f857 - Feature name table
>>> 0x23852875 - Bitmaps extension
>>> 0x0537be77 - Full disk encryption header pointer
>>> + 0x44415441 - External data file name
>>> other - Unknown header extension, can be safely
>>> ignored
>>
>> Missing a section describing the new header.
>
> The header extension that have a separate section are those that contain
> structured data. The backing file format name doesn't have one because
> it's just a string, and so is the external data file name.
Ah, fair enough, using "backing file format name" as precedence makes sense.
>
> I guess I could add a section to describe the general concept of
> external data files, if you think we have information to put there. All
> that is really required should be contained in the feature bit
> description already.
No, you've convinced me that the feature bit should be sufficient.
>
>>> @@ -450,8 +461,10 @@ Standard Cluster Descriptor:
>>> 1 - 8: Reserved (set to 0)
>>>
>>> 9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a
>>> - cluster boundary. If the offset is 0, the cluster is
>>> - unallocated.
>>> + cluster boundary. If the offset is 0 and bit 63 is clear,
>>> + the cluster is unallocated. The offset may only be 0 with
>>> + bit 63 set (indicating a host cluster offset of 0) when an
>>> + external data file is used.
>>
>> Does that mean that the value 0x00000000 is invalid for external data
>> files
>
> Not sure whether you mean the L2 entry as 0x00000000 or the offset field
> as 0x00000000 (neither of them are 32 bit like your value).
>
> In any case, 0 is a valid value for an L2 entry, it means an unallocated
> cluster. The same offset, but with bit 63 set (0x8000000000000000) is
> valid only for external data files and means an allocated cluster at
> offset 0.
>
>> and that 0x00000001 is special-cased to mean read the contents of
>> the external file (and NOT that the cluster reads as all zeroes)?
>
> No, where do you read that? This is the description of the offset
> field, and bit 0 isn't mentioned anywhere. The zero flag works the same
> way as it always does.
Arrgh. I was remembering that there were special bits on both ends of L2
entries, but then mixed up which bit you were saying had special
behavior, and wrote my paragraph thinking you used bit 0 instead of bit
63 as the way to recognize an explicit reference to external cluster 0.
So, re-reading things, bit 63 for normal qcow2 images is 0 for unused,
compressed, or needs COW; and 1 for refcount == 1. Does that mean the
bit should be cleared (for all but offset 0) for external images, or
does it mean that bit should always be set for EVERY cluster of the
external image (since there is no refcounting, every cluster in the
external image implicitly has a count of 1)? Either way, I think you
need to update the docs for bit 63 in addition to your addition to the
standard cluster descriptor 9-55.
>
>> Is bit 0 allowed to be set for any other clusters when there is an
>> external data file?
>
> Wait, what are "other clusters"? Are you assuming that we have an image
> that stores guest data both internally and externally, in some kind of a
> mixed setup?
No, I agree that external files are an all-or-none proposition within a
given qcow2: if enabled, ALL guest clusters that do not defer to a
backing file must be found at the same offset in the external file as
the guest sees. The only mixing, then, is whether we allow the use of
bit 0 to flag clusters that read as all zeroes, and/or the use of
address 0 (for unallocated) to defer to a backing file instead of the
external file (either of which means that the external file does NOT
represent the same set of data that the guest sees).
>
> The idea is that the feature bit signals that _all_ guest data is stored
> in the external data file. The offset in the L2 table always refers to
> the external file then. Only metadata remains in the qcow2 file.
Correct.
>
>> And if so, are we requiring that it only be set
>> when the external file is known to read as zero, or can we run into
>> the situation where qcow2 says the cluster reads as 0 but the host
>> file contains garbage?
>
> Hm... This is a good point. Currently it behaves just like normal qcow2
> files, but this means that the external file can contain stale data and
> the zero flag determines the content. This makes it impossible to use
> the external data file as a raw image. So I think we need to add a
> requirement to write actual zeroes to the external file there.
The same is true if we allow unallocated clusters (which can then grab
data from a backing file). The external file is usable as a raw image
ONLY if ALL clusters are allocated, AND if clusters with the 0 bit set
in the qcow2 metadata also read as zero from the external file (but
orthogonal to whether the host file is sparse or fully-allocated).
>
>> Should the external file header contain a flag
>> that states whether writes to the image should wipe vs. leave
>> unchanged a cluster in the external file when the qcow2 metadata
>> prefers to grab that cluster's contents as all-0s or by reading from
>> the backing file? There are security vs. speed implications -
>> security insists on wiping the host file to NOT leave stale data, but
>> that slows things down compared to just leaving garbage if the qcow2
>> metadata can effectively ignore those parts of the external file -
>> hence a knob may be worth exposing?
>
> If your argument is security, wouldn't the same tradeoff apply to
> internally stored data as well? zero_in_l2_slice() only adds the zero
> flag, without overwriting the data in bs->file.
With internal storage, no one can easily access the allocated cluster to
read the stale data (well, I guess they can if the use qemu-img map
--output=json and then read from the allocated offset - but that's not
easy); but once the external file exists, anyone reading from the
external file in isolation doesn't know that the image should read as
zero, and the stale data becomes much more likely to be a problem.
>
> But then, for actual security, wouldn't we need to do an explicit write
> rather that write_zeroes so that the same problem doesn't reoccur just
> one layer down? Or potentially even more specialised operations?
>
> Security is a different goal than just keeping the data file consistent
> enough to be readable as a raw image.
True - if we ever need a knob to ensure that discarded clusters are
wiped rather than preserving stale data, it's much harder than just
allowing an efficient write zeroes operation.
>
>> Should we preserve the meaning of bit 0 SOLELY for its 'reads-as-zeroes'
>> meaning, and instead make bit 1 (currently reserved) as the special
>> marker that MUST be set for clusters read from the external file,
>> keeping the two ideas orthogonal?
>
> We don't need bit 1, _all_ clusters are read from the external file.
Again, a result of my confusion on which bit was the marker for the
special handling of external cluster 0, and I see know that we don't
need any additional reserved bits in use.
>
>> Worth a mention on the reftable section that when an external file is
>> used, metadata clusters (in the qcow2 file) are still refcounted (and
>> thus, offsets in the refcount table point to the qcow2 file, not the
>> external file)?
>
> I don't see how you could interpret the spec otherwise, but if you think
> it's helpful, I guess being explicit can never hurt...
>
> Kevin
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external data files
2019-02-01 16:17 ` Eric Blake
@ 2019-02-01 16:53 ` Kevin Wolf
0 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-02-01 16:53 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-block, mreitz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 9154 bytes --]
Am 01.02.2019 um 17:17 hat Eric Blake geschrieben:
> On 2/1/19 4:21 AM, Kevin Wolf wrote:
> > Am 31.01.2019 um 19:43 hat Eric Blake geschrieben:
> >> On 1/31/19 11:55 AM, Kevin Wolf wrote:
> >>> This adds external data file to the qcow2 spec as a new incompatible
> >>> feature.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>> docs/interop/qcow2.txt | 19 ++++++++++++++++---
> >>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>
> >>
> >>> @@ -148,6 +158,7 @@ be stored. Each extension has a structure like the following:
> >>> 0x6803f857 - Feature name table
> >>> 0x23852875 - Bitmaps extension
> >>> 0x0537be77 - Full disk encryption header pointer
> >>> + 0x44415441 - External data file name
> >>> other - Unknown header extension, can be safely
> >>> ignored
> >>
> >> Missing a section describing the new header.
> >
> > The header extension that have a separate section are those that contain
> > structured data. The backing file format name doesn't have one because
> > it's just a string, and so is the external data file name.
>
> Ah, fair enough, using "backing file format name" as precedence makes sense.
>
> >
> > I guess I could add a section to describe the general concept of
> > external data files, if you think we have information to put there. All
> > that is really required should be contained in the feature bit
> > description already.
>
> No, you've convinced me that the feature bit should be sufficient.
>
> >
> >>> @@ -450,8 +461,10 @@ Standard Cluster Descriptor:
> >>> 1 - 8: Reserved (set to 0)
> >>>
> >>> 9 - 55: Bits 9-55 of host cluster offset. Must be aligned to a
> >>> - cluster boundary. If the offset is 0, the cluster is
> >>> - unallocated.
> >>> + cluster boundary. If the offset is 0 and bit 63 is clear,
> >>> + the cluster is unallocated. The offset may only be 0 with
> >>> + bit 63 set (indicating a host cluster offset of 0) when an
> >>> + external data file is used.
> >>
> >> Does that mean that the value 0x00000000 is invalid for external data
> >> files
> >
> > Not sure whether you mean the L2 entry as 0x00000000 or the offset field
> > as 0x00000000 (neither of them are 32 bit like your value).
> >
> > In any case, 0 is a valid value for an L2 entry, it means an unallocated
> > cluster. The same offset, but with bit 63 set (0x8000000000000000) is
> > valid only for external data files and means an allocated cluster at
> > offset 0.
> >
> >> and that 0x00000001 is special-cased to mean read the contents of
> >> the external file (and NOT that the cluster reads as all zeroes)?
> >
> > No, where do you read that? This is the description of the offset
> > field, and bit 0 isn't mentioned anywhere. The zero flag works the same
> > way as it always does.
>
> Arrgh. I was remembering that there were special bits on both ends of L2
> entries, but then mixed up which bit you were saying had special
> behavior, and wrote my paragraph thinking you used bit 0 instead of bit
> 63 as the way to recognize an explicit reference to external cluster 0.
>
> So, re-reading things, bit 63 for normal qcow2 images is 0 for unused,
> compressed, or needs COW; and 1 for refcount == 1. Does that mean the
> bit should be cleared (for all but offset 0) for external images, or
> does it mean that bit should always be set for EVERY cluster of the
> external image (since there is no refcounting, every cluster in the
> external image implicitly has a count of 1)? Either way, I think you
> need to update the docs for bit 63 in addition to your addition to the
> standard cluster descriptor 9-55.
It should be set for every allocated cluster because of that implicit
refcount. (This is also what the existing code automatically does, so I
didn't have to add any code to implement this.)
And because you always set it for allocated clusters in the external
file, but never for unallocated clusters, we can also use to distinguish
unallocated for allocated at offset 0. This is not really a new special
meaning, but just making use of what we already had.
But I agree that we should be more explicit in the description for bit
63 because these clusters aren't refcounted. Maybe I should also mention
somewhere that we assume an implicit refcount of 1, just in case this
matters in another place, too.
> >> Is bit 0 allowed to be set for any other clusters when there is an
> >> external data file?
> >
> > Wait, what are "other clusters"? Are you assuming that we have an image
> > that stores guest data both internally and externally, in some kind of a
> > mixed setup?
>
> No, I agree that external files are an all-or-none proposition within a
> given qcow2: if enabled, ALL guest clusters that do not defer to a
> backing file must be found at the same offset in the external file as
> the guest sees. The only mixing, then, is whether we allow the use of
> bit 0 to flag clusters that read as all zeroes, and/or the use of
> address 0 (for unallocated) to defer to a backing file instead of the
> external file (either of which means that the external file does NOT
> represent the same set of data that the guest sees).
Ok, I agree. Yes, if you have a backing file, we obviously can't make
the external data file a valid raw image until the overlay is fully
allocated. I think we need to keep zero writes consistent, though
(potentially optional, as you suggested, but it must be possible at
least).
> > The idea is that the feature bit signals that _all_ guest data is stored
> > in the external data file. The offset in the L2 table always refers to
> > the external file then. Only metadata remains in the qcow2 file.
>
> Correct.
>
> >
> >> And if so, are we requiring that it only be set
> >> when the external file is known to read as zero, or can we run into
> >> the situation where qcow2 says the cluster reads as 0 but the host
> >> file contains garbage?
> >
> > Hm... This is a good point. Currently it behaves just like normal qcow2
> > files, but this means that the external file can contain stale data and
> > the zero flag determines the content. This makes it impossible to use
> > the external data file as a raw image. So I think we need to add a
> > requirement to write actual zeroes to the external file there.
>
> The same is true if we allow unallocated clusters (which can then grab
> data from a backing file). The external file is usable as a raw image
> ONLY if ALL clusters are allocated, AND if clusters with the 0 bit set
> in the qcow2 metadata also read as zero from the external file (but
> orthogonal to whether the host file is sparse or fully-allocated).
It also works for unallocated clusters if we don't have a backing file
and the external data file is zero initialised (like a newly created
regular file would generally be). I think this is actually the most
common case.
> >> Should the external file header contain a flag
> >> that states whether writes to the image should wipe vs. leave
> >> unchanged a cluster in the external file when the qcow2 metadata
> >> prefers to grab that cluster's contents as all-0s or by reading from
> >> the backing file? There are security vs. speed implications -
> >> security insists on wiping the host file to NOT leave stale data, but
> >> that slows things down compared to just leaving garbage if the qcow2
> >> metadata can effectively ignore those parts of the external file -
> >> hence a knob may be worth exposing?
> >
> > If your argument is security, wouldn't the same tradeoff apply to
> > internally stored data as well? zero_in_l2_slice() only adds the zero
> > flag, without overwriting the data in bs->file.
>
> With internal storage, no one can easily access the allocated cluster to
> read the stale data (well, I guess they can if the use qemu-img map
> --output=json and then read from the allocated offset - but that's not
> easy); but once the external file exists, anyone reading from the
> external file in isolation doesn't know that the image should read as
> zero, and the stale data becomes much more likely to be a problem.
>
> >
> > But then, for actual security, wouldn't we need to do an explicit write
> > rather that write_zeroes so that the same problem doesn't reoccur just
> > one layer down? Or potentially even more specialised operations?
> >
> > Security is a different goal than just keeping the data file consistent
> > enough to be readable as a raw image.
>
> True - if we ever need a knob to ensure that discarded clusters are
> wiped rather than preserving stale data, it's much harder than just
> allowing an efficient write zeroes operation.
Okay, so let's declare this a non-goal for now.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [RFC PATCH 02/11] qcow2: Basic definitions for external data files
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external " Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 03/11] qcow2: Pass bs to qcow2_get_cluster_type() Kevin Wolf
` (10 subsequent siblings)
12 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
This adds basic constants, struct fields and helper function for
external data file support to the implementation.
QCOW2_INCOMPAT_MASK is not updated yet so that opening images with an
external data file still fails (we don't handle them correctly yet).
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 18 ++++++++++++++----
block/qcow2.c | 9 +++++++++
tests/qemu-iotests/031.out | 8 ++++----
tests/qemu-iotests/036.out | 4 ++--
tests/qemu-iotests/061.out | 14 +++++++-------
5 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 32cce9eee2..a242a43fe7 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -197,10 +197,12 @@ enum {
/* Incompatible feature bits */
enum {
- QCOW2_INCOMPAT_DIRTY_BITNR = 0,
- QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
- QCOW2_INCOMPAT_DIRTY = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
- QCOW2_INCOMPAT_CORRUPT = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+ QCOW2_INCOMPAT_DIRTY_BITNR = 0,
+ QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
+ QCOW2_INCOMPAT_DATA_FILE_BITNR = 2,
+ QCOW2_INCOMPAT_DIRTY = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
+ QCOW2_INCOMPAT_CORRUPT = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+ QCOW2_INCOMPAT_DATA_FILE = 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
| QCOW2_INCOMPAT_CORRUPT,
@@ -340,6 +342,8 @@ typedef struct BDRVQcow2State {
CoQueue compress_wait_queue;
int nb_compress_threads;
+
+ BdrvChild *data_file;
} BDRVQcow2State;
typedef struct Qcow2COWRegion {
@@ -457,6 +461,12 @@ typedef enum QCow2MetadataOverlap {
#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
+static inline bool has_data_file(BlockDriverState *bs)
+{
+ BDRVQcow2State *s = bs->opaque;
+ return (s->data_file != bs->file);
+}
+
static inline int64_t start_of_cluster(BDRVQcow2State *s, int64_t offset)
{
return offset & ~(s->cluster_size - 1);
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c91b92865..2b81cf839d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -73,6 +73,7 @@ typedef struct {
#define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
#define QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
#define QCOW2_EXT_MAGIC_BITMAPS 0x23852875
+#define QCOW2_EXT_MAGIC_DATA_FILE 0x44415441
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
@@ -1440,6 +1441,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
goto fail;
}
+ /* TODO Open external data file */
+ 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
* don't need header extensions, so must check here
@@ -2426,6 +2430,11 @@ int qcow2_update_header(BlockDriverState *bs)
.bit = QCOW2_INCOMPAT_CORRUPT_BITNR,
.name = "corrupt bit",
},
+ {
+ .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+ .bit = QCOW2_INCOMPAT_DATA_FILE_BITNR,
+ .name = "external data file",
+ },
{
.type = QCOW2_FEAT_TYPE_COMPATIBLE,
.bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index 7f5050b816..68a74d03b9 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -117,7 +117,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
Header extension:
@@ -150,7 +150,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
Header extension:
@@ -164,7 +164,7 @@ No errors were found on the image.
magic 0x514649fb
version 3
-backing_file_offset 0x148
+backing_file_offset 0x178
backing_file_size 0x17
cluster_bits 16
size 67108864
@@ -188,7 +188,7 @@ data 'host_device'
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
Header extension:
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index 9b009b8c15..e489b44386 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -58,7 +58,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
@@ -86,7 +86,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
*** done
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 183f7dd690..758284011b 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -26,7 +26,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
magic 0x514649fb
@@ -84,7 +84,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
magic 0x514649fb
@@ -144,7 +144,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
ERROR cluster 5 refcount=0 reference=1
@@ -199,7 +199,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
magic 0x514649fb
@@ -268,7 +268,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
read 65536/65536 bytes at offset 44040192
@@ -306,7 +306,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
ERROR cluster 5 refcount=0 reference=1
@@ -335,7 +335,7 @@ header_length 104
Header extension:
magic 0x6803f857
-length 144
+length 192
data <binary>
read 131072/131072 bytes at offset 0
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [RFC PATCH 03/11] qcow2: Pass bs to qcow2_get_cluster_type()
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 01/11] qcow2: Extend spec for external " Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 02/11] qcow2: Basic definitions " Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 04/11] qcow2: Prepare qcow2_get_cluster_type() for external data file Kevin Wolf
` (9 subsequent siblings)
12 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 3 ++-
block/qcow2-cluster.c | 37 +++++++++++++++++++------------------
block/qcow2-refcount.c | 10 +++++-----
3 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index a242a43fe7..2cb763bf11 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -508,7 +508,8 @@ static inline int64_t qcow2_vm_state_offset(BDRVQcow2State *s)
return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits);
}
-static inline QCow2ClusterType qcow2_get_cluster_type(uint64_t l2_entry)
+static inline QCow2ClusterType qcow2_get_cluster_type(BlockDriverState *bs,
+ uint64_t l2_entry)
{
if (l2_entry & QCOW_OFLAG_COMPRESSED) {
return QCOW2_CLUSTER_COMPRESSED;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 30eca26c47..7c86e3f205 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -377,8 +377,8 @@ fail:
* as contiguous. (This allows it, for example, to stop at the first compressed
* cluster which may require a different handling)
*/
-static int count_contiguous_clusters(int nb_clusters, int cluster_size,
- uint64_t *l2_slice, uint64_t stop_flags)
+static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters,
+ int cluster_size, uint64_t *l2_slice, uint64_t stop_flags)
{
int i;
QCow2ClusterType first_cluster_type;
@@ -391,7 +391,7 @@ static int count_contiguous_clusters(int nb_clusters, int cluster_size,
}
/* must be allocated */
- first_cluster_type = qcow2_get_cluster_type(first_entry);
+ first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
assert(first_cluster_type == QCOW2_CLUSTER_NORMAL ||
first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC);
@@ -409,7 +409,8 @@ static int count_contiguous_clusters(int nb_clusters, int cluster_size,
* Checks how many consecutive unallocated clusters in a given L2
* slice have the same cluster type.
*/
-static int count_contiguous_clusters_unallocated(int nb_clusters,
+static int count_contiguous_clusters_unallocated(BlockDriverState *bs,
+ int nb_clusters,
uint64_t *l2_slice,
QCow2ClusterType wanted_type)
{
@@ -419,7 +420,7 @@ static int count_contiguous_clusters_unallocated(int nb_clusters,
wanted_type == QCOW2_CLUSTER_UNALLOCATED);
for (i = 0; i < nb_clusters; i++) {
uint64_t entry = be64_to_cpu(l2_slice[i]);
- QCow2ClusterType type = qcow2_get_cluster_type(entry);
+ QCow2ClusterType type = qcow2_get_cluster_type(bs, entry);
if (type != wanted_type) {
break;
@@ -592,7 +593,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
* true */
assert(nb_clusters <= INT_MAX);
- type = qcow2_get_cluster_type(*cluster_offset);
+ type = qcow2_get_cluster_type(bs, *cluster_offset);
if (s->qcow_version < 3 && (type == QCOW2_CLUSTER_ZERO_PLAIN ||
type == QCOW2_CLUSTER_ZERO_ALLOC)) {
qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
@@ -610,14 +611,14 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
case QCOW2_CLUSTER_ZERO_PLAIN:
case QCOW2_CLUSTER_UNALLOCATED:
/* how many empty clusters ? */
- c = count_contiguous_clusters_unallocated(nb_clusters,
+ c = count_contiguous_clusters_unallocated(bs, nb_clusters,
&l2_slice[l2_index], type);
*cluster_offset = 0;
break;
case QCOW2_CLUSTER_ZERO_ALLOC:
case QCOW2_CLUSTER_NORMAL:
/* how many allocated clusters ? */
- c = count_contiguous_clusters(nb_clusters, s->cluster_size,
+ c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
&l2_slice[l2_index], QCOW_OFLAG_ZERO);
*cluster_offset &= L2E_OFFSET_MASK;
if (offset_into_cluster(s, *cluster_offset)) {
@@ -1010,14 +1011,14 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
* write, but require COW to be performed (this includes yet unallocated space,
* which must copy from the backing file)
*/
-static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
+static int count_cow_clusters(BlockDriverState *bs, int nb_clusters,
uint64_t *l2_slice, int l2_index)
{
int i;
for (i = 0; i < nb_clusters; i++) {
uint64_t l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
- QCow2ClusterType cluster_type = qcow2_get_cluster_type(l2_entry);
+ QCow2ClusterType cluster_type = qcow2_get_cluster_type(bs, l2_entry);
switch(cluster_type) {
case QCOW2_CLUSTER_NORMAL:
@@ -1162,7 +1163,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
cluster_offset = be64_to_cpu(l2_slice[l2_index]);
/* Check how many clusters are already allocated and don't need COW */
- if (qcow2_get_cluster_type(cluster_offset) == QCOW2_CLUSTER_NORMAL
+ if (qcow2_get_cluster_type(bs, cluster_offset) == QCOW2_CLUSTER_NORMAL
&& (cluster_offset & QCOW_OFLAG_COPIED))
{
/* If a specific host_offset is required, check it */
@@ -1186,7 +1187,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
/* We keep all QCOW_OFLAG_COPIED clusters */
keep_clusters =
- count_contiguous_clusters(nb_clusters, s->cluster_size,
+ count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
&l2_slice[l2_index],
QCOW_OFLAG_COPIED | QCOW_OFLAG_ZERO);
assert(keep_clusters <= nb_clusters);
@@ -1321,7 +1322,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
if (entry & QCOW_OFLAG_COMPRESSED) {
nb_clusters = 1;
} else {
- nb_clusters = count_cow_clusters(s, nb_clusters, l2_slice, l2_index);
+ nb_clusters = count_cow_clusters(bs, nb_clusters, l2_slice, l2_index);
}
/* This function is only called when there were no non-COW clusters, so if
@@ -1329,7 +1330,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
* wrong with our code. */
assert(nb_clusters > 0);
- if (qcow2_get_cluster_type(entry) == QCOW2_CLUSTER_ZERO_ALLOC &&
+ if (qcow2_get_cluster_type(bs, entry) == QCOW2_CLUSTER_ZERO_ALLOC &&
(entry & QCOW_OFLAG_COPIED) &&
(!*host_offset ||
start_of_cluster(s, *host_offset) == (entry & L2E_OFFSET_MASK)))
@@ -1349,7 +1350,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
* would be fine, too, but count_cow_clusters() above has limited
* nb_clusters already to a range of COW clusters */
preallocated_nb_clusters =
- count_contiguous_clusters(nb_clusters, s->cluster_size,
+ count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
&l2_slice[l2_index], QCOW_OFLAG_COPIED);
assert(preallocated_nb_clusters > 0);
@@ -1613,7 +1614,7 @@ static int discard_in_l2_slice(BlockDriverState *bs, uint64_t offset,
* If full_discard is true, the sector should not read back as zeroes,
* but rather fall through to the backing file.
*/
- switch (qcow2_get_cluster_type(old_l2_entry)) {
+ switch (qcow2_get_cluster_type(bs, old_l2_entry)) {
case QCOW2_CLUSTER_UNALLOCATED:
if (full_discard || !bs->backing) {
continue;
@@ -1726,7 +1727,7 @@ static int zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
* Minimize L2 changes if the cluster already reads back as
* zeroes with correct allocation.
*/
- cluster_type = qcow2_get_cluster_type(old_offset);
+ cluster_type = qcow2_get_cluster_type(bs, old_offset);
if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN ||
(cluster_type == QCOW2_CLUSTER_ZERO_ALLOC && !unmap)) {
continue;
@@ -1868,7 +1869,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
uint64_t l2_entry = be64_to_cpu(l2_slice[j]);
int64_t offset = l2_entry & L2E_OFFSET_MASK;
QCow2ClusterType cluster_type =
- qcow2_get_cluster_type(l2_entry);
+ qcow2_get_cluster_type(bs, l2_entry);
if (cluster_type != QCOW2_CLUSTER_ZERO_PLAIN &&
cluster_type != QCOW2_CLUSTER_ZERO_ALLOC) {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 6f13d470d3..05e7974d7e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1157,7 +1157,7 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
{
BDRVQcow2State *s = bs->opaque;
- switch (qcow2_get_cluster_type(l2_entry)) {
+ switch (qcow2_get_cluster_type(bs, l2_entry)) {
case QCOW2_CLUSTER_COMPRESSED:
{
int nb_csectors;
@@ -1300,7 +1300,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
entry &= ~QCOW_OFLAG_COPIED;
offset = entry & L2E_OFFSET_MASK;
- switch (qcow2_get_cluster_type(entry)) {
+ switch (qcow2_get_cluster_type(bs, entry)) {
case QCOW2_CLUSTER_COMPRESSED:
nb_csectors = ((entry >> s->csize_shift) &
s->csize_mask) + 1;
@@ -1582,7 +1582,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
for(i = 0; i < s->l2_size; i++) {
l2_entry = be64_to_cpu(l2_table[i]);
- switch (qcow2_get_cluster_type(l2_entry)) {
+ switch (qcow2_get_cluster_type(bs, l2_entry)) {
case QCOW2_CLUSTER_COMPRESSED:
/* Compressed clusters don't have QCOW_OFLAG_COPIED */
if (l2_entry & QCOW_OFLAG_COPIED) {
@@ -1633,7 +1633,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
/* Correct offsets are cluster aligned */
if (offset_into_cluster(s, offset)) {
- if (qcow2_get_cluster_type(l2_entry) ==
+ if (qcow2_get_cluster_type(bs, l2_entry) ==
QCOW2_CLUSTER_ZERO_ALLOC)
{
fprintf(stderr, "%s offset=%" PRIx64 ": Preallocated zero "
@@ -1868,7 +1868,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
for (j = 0; j < s->l2_size; j++) {
uint64_t l2_entry = be64_to_cpu(l2_table[j]);
uint64_t data_offset = l2_entry & L2E_OFFSET_MASK;
- QCow2ClusterType cluster_type = qcow2_get_cluster_type(l2_entry);
+ QCow2ClusterType cluster_type = qcow2_get_cluster_type(bs, l2_entry);
if (cluster_type == QCOW2_CLUSTER_NORMAL ||
cluster_type == QCOW2_CLUSTER_ZERO_ALLOC) {
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [RFC PATCH 04/11] qcow2: Prepare qcow2_get_cluster_type() for external data file
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (2 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 03/11] qcow2: Pass bs to qcow2_get_cluster_type() Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 05/11] qcow2: Prepare count_contiguous_clusters() " Kevin Wolf
` (8 subsequent siblings)
12 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 2cb763bf11..b17bd502f5 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -519,7 +519,15 @@ static inline QCow2ClusterType qcow2_get_cluster_type(BlockDriverState *bs,
}
return QCOW2_CLUSTER_ZERO_PLAIN;
} else if (!(l2_entry & L2E_OFFSET_MASK)) {
- return QCOW2_CLUSTER_UNALLOCATED;
+ /* Offset 0 generally means unallocated, but it is ambiguous with
+ * external data files because 0 is a valid offset there. However, all
+ * clusters in external data files always have refcount 1, so we can
+ * rely on QCOW_OFLAG_COPIED to disambiguate. */
+ if (has_data_file(bs) && (l2_entry & QCOW_OFLAG_COPIED)) {
+ return QCOW2_CLUSTER_NORMAL;
+ } else {
+ return QCOW2_CLUSTER_UNALLOCATED;
+ }
} else {
return QCOW2_CLUSTER_NORMAL;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [RFC PATCH 05/11] qcow2: Prepare count_contiguous_clusters() for external data file
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (3 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 04/11] qcow2: Prepare qcow2_get_cluster_type() for external data file Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset Kevin Wolf
` (7 subsequent siblings)
12 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
Offset 0 can be valid for normal (allocated) clusters now, so use
qcow2_get_cluster_type() instead.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2-cluster.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 7c86e3f205..73ea0f99d6 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -386,12 +386,12 @@ static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters,
uint64_t first_entry = be64_to_cpu(l2_slice[0]);
uint64_t offset = first_entry & mask;
- if (!offset) {
+ first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
+ if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) {
return 0;
}
/* must be allocated */
- first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
assert(first_cluster_type == QCOW2_CLUSTER_NORMAL ||
first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC);
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (4 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 05/11] qcow2: Prepare count_contiguous_clusters() " Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-02-18 23:13 ` Max Reitz
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 07/11] qcow2: External file I/O Kevin Wolf
` (6 subsequent siblings)
12 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
The cluster allocation code uses 0 as an invalid offset that is used in
case of errors or as "offset not yet determined". With external data
files, a host cluster offset of 0 becomes valid, though.
Define a constant INV_OFFSET (which is not cluster aligned and will
therefore never be a valid offset) that can be used for such purposes.
This removes the additional host_offset == 0 check that commit
ff52aab2df5 introduced; the confusion between an invalid offset and
(erroneous) allocation at offset 0 is removed with this change.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 2 ++
block/qcow2-cluster.c | 59 ++++++++++++++++++++-----------------------
2 files changed, 29 insertions(+), 32 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index b17bd502f5..1f87c45977 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -461,6 +461,8 @@ typedef enum QCow2MetadataOverlap {
#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
+#define INV_OFFSET (-1ULL)
+
static inline bool has_data_file(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 73ea0f99d6..4889c166e8 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1106,9 +1106,9 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
/*
* Checks how many already allocated clusters that don't require a copy on
- * write there are at the given guest_offset (up to *bytes). If
- * *host_offset is not zero, only physically contiguous clusters beginning at
- * this host offset are counted.
+ * write there are at the given guest_offset (up to *bytes). If *host_offset is
+ * not INV_OFFSET, only physically contiguous clusters beginning at this host
+ * offset are counted.
*
* Note that guest_offset may not be cluster aligned. In this case, the
* returned *host_offset points to exact byte referenced by guest_offset and
@@ -1140,8 +1140,8 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
trace_qcow2_handle_copied(qemu_coroutine_self(), guest_offset, *host_offset,
*bytes);
- assert(*host_offset == 0 || offset_into_cluster(s, guest_offset)
- == offset_into_cluster(s, *host_offset));
+ assert(*host_offset == INV_OFFSET || offset_into_cluster(s, guest_offset)
+ == offset_into_cluster(s, *host_offset));
/*
* Calculate the number of clusters to look for. We stop at L2 slice
@@ -1179,7 +1179,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t guest_offset,
goto out;
}
- if (*host_offset != 0 && !offset_matches) {
+ if (*host_offset != INV_OFFSET && !offset_matches) {
*bytes = 0;
ret = 0;
goto out;
@@ -1222,10 +1222,10 @@ out:
* contain the number of clusters that have been allocated and are contiguous
* in the image file.
*
- * If *host_offset is non-zero, it specifies the offset in the image file at
- * which the new clusters must start. *nb_clusters can be 0 on return in this
- * case if the cluster at host_offset is already in use. If *host_offset is
- * zero, the clusters can be allocated anywhere in the image file.
+ * If *host_offset is not INV_OFFSET, it specifies the offset in the image file
+ * at which the new clusters must start. *nb_clusters can be 0 on return in
+ * this case if the cluster at host_offset is already in use. If *host_offset
+ * is INV_OFFSET, the clusters can be allocated anywhere in the image file.
*
* *host_offset is updated to contain the offset into the image file at which
* the first allocated cluster starts.
@@ -1244,7 +1244,7 @@ static int do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
/* Allocate new clusters */
trace_qcow2_cluster_alloc_phys(qemu_coroutine_self());
- if (*host_offset == 0) {
+ if (*host_offset == INV_OFFSET) {
int64_t cluster_offset =
qcow2_alloc_clusters(bs, *nb_clusters * s->cluster_size);
if (cluster_offset < 0) {
@@ -1264,8 +1264,8 @@ static int do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
/*
* Allocates new clusters for an area that either is yet unallocated or needs a
- * copy on write. If *host_offset is non-zero, clusters are only allocated if
- * the new allocation can match the specified host offset.
+ * copy on write. If *host_offset is not INV_OFFSET, clusters are only
+ * allocated if the new allocation can match the specified host offset.
*
* Note that guest_offset may not be cluster aligned. In this case, the
* returned *host_offset points to exact byte referenced by guest_offset and
@@ -1293,7 +1293,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
int ret;
bool keep_old_clusters = false;
- uint64_t alloc_cluster_offset = 0;
+ uint64_t alloc_cluster_offset = INV_OFFSET;
trace_qcow2_handle_alloc(qemu_coroutine_self(), guest_offset, *host_offset,
*bytes);
@@ -1332,7 +1332,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
if (qcow2_get_cluster_type(bs, entry) == QCOW2_CLUSTER_ZERO_ALLOC &&
(entry & QCOW_OFLAG_COPIED) &&
- (!*host_offset ||
+ (*host_offset == INV_OFFSET ||
start_of_cluster(s, *host_offset) == (entry & L2E_OFFSET_MASK)))
{
int preallocated_nb_clusters;
@@ -1364,9 +1364,10 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
- if (!alloc_cluster_offset) {
+ if (alloc_cluster_offset == INV_OFFSET) {
/* Allocate, if necessary at a given offset in the image file */
- alloc_cluster_offset = start_of_cluster(s, *host_offset);
+ alloc_cluster_offset = *host_offset == INV_OFFSET ? INV_OFFSET :
+ start_of_cluster(s, *host_offset);
ret = do_alloc_cluster_offset(bs, guest_offset, &alloc_cluster_offset,
&nb_clusters);
if (ret < 0) {
@@ -1379,16 +1380,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
return 0;
}
- /* !*host_offset would overwrite the image header and is reserved for
- * "no host offset preferred". If 0 was a valid host offset, it'd
- * trigger the following overlap check; do that now to avoid having an
- * invalid value in *host_offset. */
- if (!alloc_cluster_offset) {
- ret = qcow2_pre_write_overlap_check(bs, 0, alloc_cluster_offset,
- nb_clusters * s->cluster_size);
- assert(ret < 0);
- goto fail;
- }
+ assert(alloc_cluster_offset != INV_OFFSET);
}
/*
@@ -1480,14 +1472,14 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
again:
start = offset;
remaining = *bytes;
- cluster_offset = 0;
- *host_offset = 0;
+ cluster_offset = INV_OFFSET;
+ *host_offset = INV_OFFSET;
cur_bytes = 0;
*m = NULL;
while (true) {
- if (!*host_offset) {
+ if (*host_offset == INV_OFFSET && cluster_offset != INV_OFFSET) {
*host_offset = start_of_cluster(s, cluster_offset);
}
@@ -1495,7 +1487,10 @@ again:
start += cur_bytes;
remaining -= cur_bytes;
- cluster_offset += cur_bytes;
+
+ if (cluster_offset != INV_OFFSET) {
+ cluster_offset += cur_bytes;
+ }
if (remaining == 0) {
break;
@@ -1567,7 +1562,7 @@ again:
*bytes -= remaining;
assert(*bytes > 0);
- assert(*host_offset != 0);
+ assert(*host_offset != INV_OFFSET);
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset Kevin Wolf
@ 2019-02-18 23:13 ` Max Reitz
2019-02-19 8:45 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-18 23:13 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]
On 31.01.19 18:55, Kevin Wolf wrote:
> The cluster allocation code uses 0 as an invalid offset that is used in
> case of errors or as "offset not yet determined". With external data
> files, a host cluster offset of 0 becomes valid, though.
>
> Define a constant INV_OFFSET (which is not cluster aligned and will
> therefore never be a valid offset) that can be used for such purposes.
>
> This removes the additional host_offset == 0 check that commit
> ff52aab2df5 introduced; the confusion between an invalid offset and
> (erroneous) allocation at offset 0 is removed with this change.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qcow2.h | 2 ++
> block/qcow2-cluster.c | 59 ++++++++++++++++++++-----------------------
> 2 files changed, 29 insertions(+), 32 deletions(-)
qcow2_get_cluster_offset() still returns 0 for unallocated clusters.
(And qcow2_co_block_status() tests for that, so it would never report a
valid offset for the first cluster in an externally allocated qcow2 file.)
qcow2_alloc_compressed_cluster_offset() should return INV_OFFSET on
error (yeah, there are no compressed clusters in external files, but
this seems like the right thing to do still).
(And there are cases like qcow2_co_preadv(), where cluster_offset is
initialized to 0 -- it doesn't make a difference what it's initialized
to (it's just to silence the compiler, I suppose), but it should still
use this new constant now. I think.)
Now bikeshedding begins: Also, s->free_byte_offset is initialized to 0
and that is the expected value for "nothing allocated yet". I think I'd
prefer all of the qocw2 code to use a common invalidity constant, even
thought it would make things like that more complicated. But then we
might get into the metadata territory (how bad is it that
s->bitmap_directory_offset too is 0 when there is no directory?),
because compressed clusters are not allowed in external files, just like
metadata is not...
So my bikeshedding result is "I think it would be nice if all of the
qcow2 code made use of this constant, but it may also be pretty stupid
to enforce that now."
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
2019-02-18 23:13 ` Max Reitz
@ 2019-02-19 8:45 ` Kevin Wolf
2019-02-22 14:09 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-19 8:45 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]
Am 19.02.2019 um 00:13 hat Max Reitz geschrieben:
> On 31.01.19 18:55, Kevin Wolf wrote:
> > The cluster allocation code uses 0 as an invalid offset that is used in
> > case of errors or as "offset not yet determined". With external data
> > files, a host cluster offset of 0 becomes valid, though.
> >
> > Define a constant INV_OFFSET (which is not cluster aligned and will
> > therefore never be a valid offset) that can be used for such purposes.
> >
> > This removes the additional host_offset == 0 check that commit
> > ff52aab2df5 introduced; the confusion between an invalid offset and
> > (erroneous) allocation at offset 0 is removed with this change.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > block/qcow2.h | 2 ++
> > block/qcow2-cluster.c | 59 ++++++++++++++++++++-----------------------
> > 2 files changed, 29 insertions(+), 32 deletions(-)
>
> qcow2_get_cluster_offset() still returns 0 for unallocated clusters.
> (And qcow2_co_block_status() tests for that, so it would never report a
> valid offset for the first cluster in an externally allocated qcow2 file.)
I think the bug here is in qcow2_get_cluster_offset(). It shouldn't look
at cluster_offset, but at ret if it wants to know the allocation status.
So I think this needs to become something like:
if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC) &&
!s->crypto) {
...
}
> qcow2_alloc_compressed_cluster_offset() should return INV_OFFSET on
> error (yeah, there are no compressed clusters in external files, but
> this seems like the right thing to do still).
Ok, makes sense.
> (And there are cases like qcow2_co_preadv(), where cluster_offset is
> initialized to 0 -- it doesn't make a difference what it's initialized
> to (it's just to silence the compiler, I suppose), but it should still
> use this new constant now. I think.)
I don't think I would change places where cluster_offset is never used
at all or never used alone without checking the cluster type, too.
qcow2_get_cluster_offset() still returns 0 for unallocated and
non-preallocated zero clusters, and I think that's fine because it also
returns the cluster type, which is information about whether the offset
is even valid.
In theory, it shouldn't matter at all if we return 0, INV_OFFSET or 42
there, but in practice I'd bet neither money nor production images on
this. If it ain't broke, don't fix it.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset
2019-02-19 8:45 ` Kevin Wolf
@ 2019-02-22 14:09 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2019-02-22 14:09 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3706 bytes --]
On 19.02.19 09:45, Kevin Wolf wrote:
> Am 19.02.2019 um 00:13 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> The cluster allocation code uses 0 as an invalid offset that is used in
>>> case of errors or as "offset not yet determined". With external data
>>> files, a host cluster offset of 0 becomes valid, though.
>>>
>>> Define a constant INV_OFFSET (which is not cluster aligned and will
>>> therefore never be a valid offset) that can be used for such purposes.
>>>
>>> This removes the additional host_offset == 0 check that commit
>>> ff52aab2df5 introduced; the confusion between an invalid offset and
>>> (erroneous) allocation at offset 0 is removed with this change.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> block/qcow2.h | 2 ++
>>> block/qcow2-cluster.c | 59 ++++++++++++++++++++-----------------------
>>> 2 files changed, 29 insertions(+), 32 deletions(-)
>>
>> qcow2_get_cluster_offset() still returns 0 for unallocated clusters.
>> (And qcow2_co_block_status() tests for that, so it would never report a
>> valid offset for the first cluster in an externally allocated qcow2 file.)
>
> I think the bug here is in qcow2_get_cluster_offset().
You mean qcow2_co_block_status()?
> It shouldn't look
> at cluster_offset, but at ret if it wants to know the allocation status.
> So I think this needs to become something like:
>
> if ((ret == QCOW2_CLUSTER_NORMAL || ret == QCOW2_CLUSTER_ZERO_ALLOC) &&
> !s->crypto) {
> ...
> }
I don't think that, because it doesn't want to know the allocation
status. It wants to know whether it can return valid map information,
which it can if (1) qcow2_get_cluster_offset() returned a valid offset,
and (2) the data is represented in plain text (i.e. not compressed or
encrypted).
OTOH maybe having a whitelist instead of a blacklist would indeed be
more safe, in a sense. But first, this isn't a pure whitelist, because
it still has to check "!s->crypto". Second, it isn't like allocated
zero clusters store the data the same way it's seen in the guest. So
even the whitelist part feels a bit weird to me.
All in all, the way it is right now makes more sense to me.
>> qcow2_alloc_compressed_cluster_offset() should return INV_OFFSET on
>> error (yeah, there are no compressed clusters in external files, but
>> this seems like the right thing to do still).
>
> Ok, makes sense.
>
>> (And there are cases like qcow2_co_preadv(), where cluster_offset is
>> initialized to 0 -- it doesn't make a difference what it's initialized
>> to (it's just to silence the compiler, I suppose), but it should still
>> use this new constant now. I think.)
>
> I don't think I would change places where cluster_offset is never used
> at all or never used alone without checking the cluster type, too.
OK.
> qcow2_get_cluster_offset() still returns 0 for unallocated and
> non-preallocated zero clusters, and I think that's fine because it also
> returns the cluster type, which is information about whether the offset
> is even valid.
>
> In theory, it shouldn't matter at all if we return 0, INV_OFFSET or 42
> there, but in practice I'd bet neither money nor production images on
> this. If it ain't broke, don't fix it.
I don't see how an "organic growth" code base which sometimes uses 0 and
sometimes INV_OFFSET for invalid offsets is any more trustworthy, but
whatever. I'm in a position where I don't have to learn the qcow2 code
from scratch but instead would have to review your changes, so I won't
complain further.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [RFC PATCH 07/11] qcow2: External file I/O
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (5 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 06/11] qcow2: Don't assume 0 is an invalid cluster offset Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-02-18 23:36 ` Max Reitz
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure Kevin Wolf
` (5 subsequent siblings)
12 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
This changes the qcow2 implementation to direct all guest data I/O to
s->data_file rather than bs->file, while metadata I/O still uses
bs->file. At the moment, this is still always the same, but soon we'll
add options to set s->data_file to an external data file.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 2 +-
block/qcow2-bitmap.c | 7 ++++---
block/qcow2-cache.c | 6 +++---
block/qcow2-cluster.c | 46 +++++++++++++++++++++++++++++++++++-------
block/qcow2-refcount.c | 30 +++++++++++++++++++--------
block/qcow2-snapshot.c | 7 ++++---
block/qcow2.c | 39 +++++++++++++++++++++++++----------
7 files changed, 101 insertions(+), 36 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 1f87c45977..c161970882 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -620,7 +620,7 @@ void qcow2_process_discards(BlockDriverState *bs, int ret);
int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
int64_t size);
int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
- int64_t size);
+ int64_t size, bool data_file);
int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b946301429..9f42ce13cb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -778,7 +778,8 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
* directory in-place (actually, turn-off the extension), which is checked
* in qcow2_check_metadata_overlap() */
ret = qcow2_pre_write_overlap_check(
- bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size);
+ bs, in_place ? QCOW2_OL_BITMAP_DIRECTORY : 0, dir_offset, dir_size,
+ false);
if (ret < 0) {
goto fail;
}
@@ -1148,7 +1149,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
memset(buf + write_size, 0, s->cluster_size - write_size);
}
- ret = qcow2_pre_write_overlap_check(bs, 0, off, s->cluster_size);
+ ret = qcow2_pre_write_overlap_check(bs, 0, off, s->cluster_size, false);
if (ret < 0) {
error_setg_errno(errp, -ret, "Qcow2 overlap check failed");
goto fail;
@@ -1216,7 +1217,7 @@ static int store_bitmap(BlockDriverState *bs, Qcow2Bitmap *bm, Error **errp)
}
ret = qcow2_pre_write_overlap_check(bs, 0, tb_offset,
- tb_size * sizeof(tb[0]));
+ tb_size * sizeof(tb[0]), false);
if (ret < 0) {
error_setg_errno(errp, -ret, "Qcow2 overlap check failed");
goto fail;
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index d9dafa31e5..df02e7b20a 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -205,13 +205,13 @@ static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i)
if (c == s->refcount_block_cache) {
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_REFCOUNT_BLOCK,
- c->entries[i].offset, c->table_size);
+ c->entries[i].offset, c->table_size, false);
} else if (c == s->l2_table_cache) {
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
- c->entries[i].offset, c->table_size);
+ c->entries[i].offset, c->table_size, false);
} else {
ret = qcow2_pre_write_overlap_check(bs, 0,
- c->entries[i].offset, c->table_size);
+ c->entries[i].offset, c->table_size, false);
}
if (ret < 0) {
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4889c166e8..fbd967c5a8 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -153,7 +153,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
/* the L1 position has not yet been updated, so these clusters must
* indeed be completely free */
ret = qcow2_pre_write_overlap_check(bs, 0, new_l1_table_offset,
- new_l1_size2);
+ new_l1_size2, false);
if (ret < 0) {
goto fail;
}
@@ -238,7 +238,7 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
}
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,
- s->l1_table_offset + 8 * l1_start_index, sizeof(buf));
+ s->l1_table_offset + 8 * l1_start_index, sizeof(buf), false);
if (ret < 0) {
return ret;
}
@@ -487,6 +487,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
unsigned offset_in_cluster,
QEMUIOVector *qiov)
{
+ BDRVQcow2State *s = bs->opaque;
int ret;
if (qiov->size == 0) {
@@ -494,13 +495,13 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
}
ret = qcow2_pre_write_overlap_check(bs, 0,
- cluster_offset + offset_in_cluster, qiov->size);
+ cluster_offset + offset_in_cluster, qiov->size, true);
if (ret < 0) {
return ret;
}
BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
- ret = bdrv_co_pwritev(bs->file, cluster_offset + offset_in_cluster,
+ ret = bdrv_co_pwritev(s->data_file, cluster_offset + offset_in_cluster,
qiov->size, qiov, 0);
if (ret < 0) {
return ret;
@@ -604,6 +605,14 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
}
switch (type) {
case QCOW2_CLUSTER_COMPRESSED:
+ if (has_data_file(bs)) {
+ qcow2_signal_corruption(bs, true, -1, -1, "Compressed cluster "
+ "entry found in image with external data "
+ "file (L2 offset: %#" PRIx64 ", L2 index: "
+ "%#x)", l2_offset, l2_index);
+ ret = -EIO;
+ goto fail;
+ }
/* Compressed clusters can only be processed one by one */
c = 1;
*cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK;
@@ -630,6 +639,17 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
ret = -EIO;
goto fail;
}
+ if (has_data_file(bs) && *cluster_offset != offset - offset_in_cluster)
+ {
+ qcow2_signal_corruption(bs, true, -1, -1,
+ "External data file host cluster offset %#"
+ PRIx64 " does not match guest cluster "
+ "offset: %#" PRIx64
+ ", L2 index: %#x)", *cluster_offset,
+ offset - offset_in_cluster, l2_index);
+ ret = -EIO;
+ goto fail;
+ }
break;
default:
abort();
@@ -753,6 +773,10 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
int64_t cluster_offset;
int nb_csectors;
+ if (has_data_file(bs)) {
+ return 0;
+ }
+
ret = get_cluster_table(bs, offset, &l2_slice, &l2_index);
if (ret < 0) {
return 0;
@@ -1242,6 +1266,13 @@ static int do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
trace_qcow2_do_alloc_clusters_offset(qemu_coroutine_self(), guest_offset,
*host_offset, *nb_clusters);
+ if (has_data_file(bs)) {
+ assert(*host_offset == INV_OFFSET ||
+ *host_offset == start_of_cluster(s, guest_offset));
+ *host_offset = start_of_cluster(s, guest_offset);
+ return 0;
+ }
+
/* Allocate new clusters */
trace_qcow2_cluster_alloc_phys(qemu_coroutine_self());
if (*host_offset == INV_OFFSET) {
@@ -1918,7 +1949,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
}
ret = qcow2_pre_write_overlap_check(bs, 0, offset,
- s->cluster_size);
+ s->cluster_size, true);
if (ret < 0) {
if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN) {
qcow2_free_clusters(bs, offset, s->cluster_size,
@@ -1927,7 +1958,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
goto fail;
}
- ret = bdrv_pwrite_zeroes(bs->file, offset, s->cluster_size, 0);
+ ret = bdrv_pwrite_zeroes(s->data_file, offset,
+ s->cluster_size, 0);
if (ret < 0) {
if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN) {
qcow2_free_clusters(bs, offset, s->cluster_size,
@@ -1954,7 +1986,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
if (l2_dirty) {
ret = qcow2_pre_write_overlap_check(
bs, QCOW2_OL_INACTIVE_L2 | QCOW2_OL_ACTIVE_L2,
- slice_offset, slice_size2);
+ slice_offset, slice_size2, false);
if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 05e7974d7e..79045497c3 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1157,6 +1157,11 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
{
BDRVQcow2State *s = bs->opaque;
+ if (has_data_file(bs)) {
+ /* TODO Pass through discard request to s->data_file */
+ return;
+ }
+
switch (qcow2_get_cluster_type(bs, l2_entry)) {
case QCOW2_CLUSTER_COMPRESSED:
{
@@ -1649,7 +1654,7 @@ static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res,
l2_table[i] = cpu_to_be64(l2_entry);
ret = qcow2_pre_write_overlap_check(bs,
QCOW2_OL_ACTIVE_L2 | QCOW2_OL_INACTIVE_L2,
- l2e_offset, sizeof(uint64_t));
+ l2e_offset, sizeof(uint64_t), false);
if (ret < 0) {
fprintf(stderr, "ERROR: Overlap check failed\n");
res->check_errors++;
@@ -1898,7 +1903,8 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
if (l2_dirty) {
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L2,
- l2_offset, s->cluster_size);
+ l2_offset, s->cluster_size,
+ false);
if (ret < 0) {
fprintf(stderr, "ERROR: Could not write L2 table; metadata "
"overlap check failed: %s\n", strerror(-ret));
@@ -2366,7 +2372,7 @@ write_refblocks:
}
ret = qcow2_pre_write_overlap_check(bs, 0, refblock_offset,
- s->cluster_size);
+ s->cluster_size, false);
if (ret < 0) {
fprintf(stderr, "ERROR writing refblock: %s\n", strerror(-ret));
goto fail;
@@ -2417,7 +2423,8 @@ write_refblocks:
}
ret = qcow2_pre_write_overlap_check(bs, 0, reftable_offset,
- reftable_size * sizeof(uint64_t));
+ reftable_size * sizeof(uint64_t),
+ false);
if (ret < 0) {
fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret));
goto fail;
@@ -2751,10 +2758,15 @@ QEMU_BUILD_BUG_ON(QCOW2_OL_MAX_BITNR != ARRAY_SIZE(metadata_ol_names));
* overlaps; or a negative value (-errno) on error.
*/
int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
- int64_t size)
+ int64_t size, bool data_file)
{
- int ret = qcow2_check_metadata_overlap(bs, ign, offset, size);
+ int ret;
+
+ if (data_file && has_data_file(bs)) {
+ return 0;
+ }
+ ret = qcow2_check_metadata_overlap(bs, ign, offset, size);
if (ret < 0) {
return ret;
} else if (ret > 0) {
@@ -2855,7 +2867,8 @@ static int flush_refblock(BlockDriverState *bs, uint64_t **reftable,
if (reftable_index < *reftable_size && (*reftable)[reftable_index]) {
offset = (*reftable)[reftable_index];
- ret = qcow2_pre_write_overlap_check(bs, 0, offset, s->cluster_size);
+ ret = qcow2_pre_write_overlap_check(bs, 0, offset, s->cluster_size,
+ false);
if (ret < 0) {
error_setg_errno(errp, -ret, "Overlap check failed");
return ret;
@@ -3121,7 +3134,8 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
/* Write the new reftable */
ret = qcow2_pre_write_overlap_check(bs, 0, new_reftable_offset,
- new_reftable_size * sizeof(uint64_t));
+ new_reftable_size * sizeof(uint64_t),
+ false);
if (ret < 0) {
error_setg_errno(errp, -ret, "Overlap check failed");
goto done;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index bb6a5b7516..b0897886c4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -184,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
/* The snapshot list position has not yet been updated, so these clusters
* must indeed be completely free */
- ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size);
+ ret = qcow2_pre_write_overlap_check(bs, 0, offset, snapshots_size, false);
if (ret < 0) {
goto fail;
}
@@ -394,7 +394,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
}
ret = qcow2_pre_write_overlap_check(bs, 0, sn->l1_table_offset,
- s->l1_size * sizeof(uint64_t));
+ s->l1_size * sizeof(uint64_t), false);
if (ret < 0) {
goto fail;
}
@@ -533,7 +533,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
}
ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_ACTIVE_L1,
- s->l1_table_offset, cur_l1_bytes);
+ s->l1_table_offset, cur_l1_bytes,
+ false);
if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 2b81cf839d..ac9934b3ed 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -140,7 +140,7 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen,
/* Zero fill remaining space in cluster so it has predictable
* content in case of future spec changes */
clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;
- assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen) == 0);
+ assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen, false) == 0);
ret = bdrv_pwrite_zeroes(bs->file,
ret + headerlen,
clusterlen - headerlen, 0);
@@ -1951,7 +1951,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
*/
if (!cluster_data) {
cluster_data =
- qemu_try_blockalign(bs->file->bs,
+ qemu_try_blockalign(s->data_file->bs,
QCOW_MAX_CRYPT_CLUSTERS
* s->cluster_size);
if (cluster_data == NULL) {
@@ -1967,7 +1967,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
qemu_co_mutex_unlock(&s->lock);
- ret = bdrv_co_preadv(bs->file,
+ ret = bdrv_co_preadv(s->data_file,
cluster_offset + offset_in_cluster,
cur_bytes, &hd_qiov, 0);
qemu_co_mutex_lock(&s->lock);
@@ -2126,7 +2126,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
}
ret = qcow2_pre_write_overlap_check(bs, 0,
- cluster_offset + offset_in_cluster, cur_bytes);
+ cluster_offset + offset_in_cluster, cur_bytes, true);
if (ret < 0) {
goto fail;
}
@@ -2140,7 +2140,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
trace_qcow2_writev_data(qemu_coroutine_self(),
cluster_offset + offset_in_cluster);
- ret = bdrv_co_pwritev(bs->file,
+ ret = bdrv_co_pwritev(s->data_file,
cluster_offset + offset_in_cluster,
cur_bytes, &hd_qiov, 0);
qemu_co_mutex_lock(&s->lock);
@@ -3366,7 +3366,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
goto out;
case QCOW2_CLUSTER_NORMAL:
- child = bs->file;
+ child = s->data_file;
copy_offset += offset_into_cluster(s, src_offset);
if ((copy_offset & 511) != 0) {
ret = -EIO;
@@ -3436,14 +3436,14 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
assert((cluster_offset & 511) == 0);
ret = qcow2_pre_write_overlap_check(bs, 0,
- cluster_offset + offset_in_cluster, cur_bytes);
+ cluster_offset + offset_in_cluster, cur_bytes, true);
if (ret < 0) {
goto fail;
}
qemu_co_mutex_unlock(&s->lock);
ret = bdrv_co_copy_range_to(src, src_offset,
- bs->file,
+ s->data_file,
cluster_offset + offset_in_cluster,
cur_bytes, read_flags, write_flags);
qemu_co_mutex_lock(&s->lock);
@@ -3598,6 +3598,16 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
int64_t old_file_size, new_file_size;
uint64_t nb_new_data_clusters, nb_new_l2_tables;
+ /* With a data file, preallocation means just allocating the metadata
+ * and forwarding the truncate request to the data file */
+ if (has_data_file(bs)) {
+ ret = preallocate_co(bs, old_length, offset);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Preallocation failed");
+ goto fail;
+ }
+ }
+
old_file_size = bdrv_getlength(bs->file->bs);
if (old_file_size < 0) {
error_setg_errno(errp, -old_file_size,
@@ -3706,6 +3716,13 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
bs->total_sectors = offset / BDRV_SECTOR_SIZE;
+ if (has_data_file(bs)) {
+ ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
+ if (ret < 0) {
+ goto fail;
+ }
+ }
+
/* write updated header.size */
offset = cpu_to_be64(offset);
ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
@@ -3963,7 +3980,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
}
cluster_offset &= s->cluster_offset_mask;
- ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
+ ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len, true);
qemu_co_mutex_unlock(&s->lock);
if (ret < 0) {
goto fail;
@@ -3975,8 +3992,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
};
qemu_iovec_init_external(&hd_qiov, &iov, 1);
- BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
- ret = bdrv_co_pwritev(bs->file, cluster_offset, out_len, &hd_qiov, 0);
+ BLKDBG_EVENT(s->data_file, BLKDBG_WRITE_COMPRESSED);
+ ret = bdrv_co_pwritev(s->data_file, cluster_offset, out_len, &hd_qiov, 0);
if (ret < 0) {
goto fail;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 07/11] qcow2: External file I/O
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 07/11] qcow2: External file I/O Kevin Wolf
@ 2019-02-18 23:36 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2019-02-18 23:36 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]
On 31.01.19 18:55, Kevin Wolf wrote:
> This changes the qcow2 implementation to direct all guest data I/O to
> s->data_file rather than bs->file, while metadata I/O still uses
> bs->file. At the moment, this is still always the same, but soon we'll
> add options to set s->data_file to an external data file.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qcow2.h | 2 +-
> block/qcow2-bitmap.c | 7 ++++---
> block/qcow2-cache.c | 6 +++---
> block/qcow2-cluster.c | 46 +++++++++++++++++++++++++++++++++++-------
> block/qcow2-refcount.c | 30 +++++++++++++++++++--------
> block/qcow2-snapshot.c | 7 ++++---
> block/qcow2.c | 39 +++++++++++++++++++++++++----------
> 7 files changed, 101 insertions(+), 36 deletions(-)
[...]
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2b81cf839d..ac9934b3ed 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
[...]
> @@ -3598,6 +3598,16 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
> int64_t old_file_size, new_file_size;
> uint64_t nb_new_data_clusters, nb_new_l2_tables;
>
> + /* With a data file, preallocation means just allocating the metadata
> + * and forwarding the truncate request to the data file */
That's true, but...
> + if (has_data_file(bs)) {
> + ret = preallocate_co(bs, old_length, offset);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "Preallocation failed");
> + goto fail;
> + }
...without a break here we're still going to preallocate bs->file as before.
Max
> + }
> +
> old_file_size = bdrv_getlength(bs->file->bs);
> if (old_file_size < 0) {
> error_setg_errno(errp, -old_file_size,
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (6 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 07/11] qcow2: External file I/O Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 18:44 ` Eric Blake
2019-02-18 23:57 ` Max Reitz
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 09/11] qcow2: Creating images with external data file Kevin Wolf
` (4 subsequent siblings)
12 siblings, 2 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure Kevin Wolf
@ 2019-01-31 18:44 ` Eric Blake
2019-02-01 10:30 ` Kevin Wolf
2019-02-18 23:57 ` Max Reitz
1 sibling, 1 reply; 46+ messages in thread
From: Eric Blake @ 2019-01-31 18:44 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: mreitz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-01-31 18:44 ` Eric Blake
@ 2019-02-01 10:30 ` Kevin Wolf
0 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-02-01 10:30 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-block, mreitz, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure Kevin Wolf
2019-01-31 18:44 ` Eric Blake
@ 2019-02-18 23:57 ` Max Reitz
2019-02-19 8:51 ` Kevin Wolf
1 sibling, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-18 23:57 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-02-18 23:57 ` Max Reitz
@ 2019-02-19 8:51 ` Kevin Wolf
2019-02-22 14:12 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-19 8:51 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-02-19 8:51 ` Kevin Wolf
@ 2019-02-22 14:12 ` Max Reitz
2019-02-22 15:38 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-22 14:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2935 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-02-22 14:12 ` Max Reitz
@ 2019-02-22 15:38 ` Kevin Wolf
2019-02-22 15:45 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-22 15:38 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3547 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure
2019-02-22 15:38 ` Kevin Wolf
@ 2019-02-22 15:45 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2019-02-22 15:45 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3650 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [RFC PATCH 09/11] qcow2: Creating images with external data file
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (7 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 08/11] qcow2: Add basic data-file infrastructure Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image Kevin Wolf
` (3 subsequent siblings)
12 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
This adds a .bdrv_create option to use an external data file.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 1 +
include/block/block_int.h | 1 +
block/qcow2.c | 26 ++++++++++++++++++++++++++
3 files changed, 28 insertions(+)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fc46396079..060df28797 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3980,6 +3980,7 @@
##
{ 'struct': 'BlockdevCreateOptionsQcow2',
'data': { 'file': 'BlockdevRef',
+ '*data-file': 'BlockdevRef',
'size': 'size',
'*version': 'BlockdevQcow2Version',
'*backing-file': 'str',
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622216..c5b70b3109 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -56,6 +56,7 @@
#define BLOCK_OPT_NOCOW "nocow"
#define BLOCK_OPT_OBJECT_SIZE "object_size"
#define BLOCK_OPT_REFCOUNT_BITS "refcount_bits"
+#define BLOCK_OPT_DATA_FILE "data_file"
#define BLOCK_PROBE_BUF_SIZE 512
diff --git a/block/qcow2.c b/block/qcow2.c
index 376232d3f0..6cf862e8b9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2855,6 +2855,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
*/
BlockBackend *blk = NULL;
BlockDriverState *bs = NULL;
+ BlockDriverState *data_bs = NULL;
QCowHeader *header;
size_t cluster_size;
int version;
@@ -2951,6 +2952,20 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
}
refcount_order = ctz32(qcow2_opts->refcount_bits);
+ if (qcow2_opts->data_file) {
+ if (version < 3) {
+ error_setg(errp, "External data files are only supported with "
+ "compatibility level 1.1 and above (use version=v3 or "
+ "greater)");
+ ret = -EINVAL;
+ goto out;
+ }
+ data_bs = bdrv_open_blockdev_ref(qcow2_opts->data_file, errp);
+ if (bs == NULL) {
+ ret = -EIO;
+ goto out;
+ }
+ }
/* Create BlockBackend to write to the image */
blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
@@ -3002,6 +3017,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
header->compatible_features |=
cpu_to_be64(QCOW2_COMPAT_LAZY_REFCOUNTS);
}
+ if (data_bs) {
+ header->incompatible_features |=
+ cpu_to_be64(QCOW2_INCOMPAT_DATA_FILE);
+ }
ret = blk_pwrite(blk, 0, header, cluster_size, 0);
g_free(header);
@@ -3032,6 +3051,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
options = qdict_new();
qdict_put_str(options, "driver", "qcow2");
qdict_put_str(options, "file", bs->node_name);
+ if (data_bs) {
+ qdict_put_str(options, "data-file", data_bs->node_name);
+ }
blk = blk_new_open(NULL, NULL, options,
BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH,
&local_err);
@@ -3117,6 +3139,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
options = qdict_new();
qdict_put_str(options, "driver", "qcow2");
qdict_put_str(options, "file", bs->node_name);
+ if (data_bs) {
+ qdict_put_str(options, "data-file", data_bs->node_name);
+ }
blk = blk_new_open(NULL, NULL, options,
BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO,
&local_err);
@@ -3130,6 +3155,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
out:
blk_unref(blk);
bdrv_unref(bs);
+ bdrv_unref(data_bs);
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (8 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 09/11] qcow2: Creating images with external data file Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-01-31 22:39 ` Nir Soffer
2019-02-19 0:18 ` Max Reitz
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2 Kevin Wolf
` (2 subsequent siblings)
12 siblings, 2 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
Rather than requiring that the external data file node is passed
explicitly when creating the qcow2 node, store the filename in the
designated header extension during .bdrv_create and read it from there
as a default during .bdrv_open.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 1 +
block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
tests/qemu-iotests/082.out | 27 +++++++++++++++
3 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index e2114900b4..a1e2600643 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -341,6 +341,7 @@ typedef struct BDRVQcow2State {
* override) */
char *image_backing_file;
char *image_backing_format;
+ char *image_data_file;
CoQueue compress_wait_queue;
int nb_compress_threads;
diff --git a/block/qcow2.c b/block/qcow2.c
index 6cf862e8b9..4959bf16a4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
#endif
break;
+ case QCOW2_EXT_MAGIC_DATA_FILE:
+ {
+ s->image_data_file = g_malloc0(ext.len + 1);
+ ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "ERROR: Could not data file name");
+ return ret;
+ }
+#ifdef DEBUG_EXT
+ printf("Qcow2: Got external data file %s\n", s->image_data_file);
+#endif
+ break;
+ }
+
default:
/* unknown magic - save it in case we need to rewrite the header */
/* If you add a new feature, make sure to also update the fast
@@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* 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);
+ &child_file, false, &local_err);
+ if (!s->data_file) {
+ if (s->image_data_file) {
+ error_free(local_err);
+ local_err = NULL;
+ s->data_file = bdrv_open_child(s->image_data_file, options,
+ "data-file", bs, &child_file,
+ false, errp);
+ } else {
+ error_propagate(errp, local_err);
+ }
+ }
if (!s->data_file) {
ret = -EINVAL;
goto fail;
@@ -1627,6 +1652,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
return ret;
fail:
+ g_free(s->image_data_file);
if (has_data_file(bs)) {
bdrv_unref_child(bs, s->data_file);
}
@@ -2246,6 +2272,7 @@ static void qcow2_close(BlockDriverState *bs)
g_free(s->unknown_header_fields);
cleanup_unknown_header_ext(bs);
+ g_free(s->image_data_file);
g_free(s->image_backing_file);
g_free(s->image_backing_format);
@@ -2422,6 +2449,19 @@ int qcow2_update_header(BlockDriverState *bs)
buflen -= ret;
}
+ /* External data file header extension */
+ if (has_data_file(bs) && s->image_data_file) {
+ ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DATA_FILE,
+ s->image_data_file, strlen(s->image_data_file),
+ buflen);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ buf += ret;
+ buflen -= ret;
+ }
+
/* Full disk encryption header pointer extension */
if (s->crypto_header.offset != 0) {
s->crypto_header.offset = cpu_to_be64(s->crypto_header.offset);
@@ -3166,6 +3206,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
QDict *qdict;
Visitor *v;
BlockDriverState *bs = NULL;
+ BlockDriverState *data_bs = NULL;
Error *local_err = NULL;
const char *val;
int ret;
@@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
goto finish;
}
+ /* Create and open an external data file (protocol layer) */
+ val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
+ if (val) {
+ ret = bdrv_create_file(val, opts, errp);
+ if (ret < 0) {
+ goto finish;
+ }
+
+ data_bs = bdrv_open(val, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+ errp);
+ if (data_bs == NULL) {
+ ret = -EIO;
+ goto finish;
+ }
+
+ qdict_del(qdict, BLOCK_OPT_DATA_FILE);
+ qdict_put_str(qdict, "data-file", data_bs->node_name);
+ }
+
/* Set 'driver' and 'node' options */
qdict_put_str(qdict, "driver", "qcow2");
qdict_put_str(qdict, "file", bs->node_name);
@@ -3263,6 +3324,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
finish:
qobject_unref(qdict);
bdrv_unref(bs);
+ bdrv_unref(data_bs);
qapi_free_BlockdevCreateOptions(create_options);
return ret;
}
@@ -4943,6 +5005,11 @@ static QemuOptsList qcow2_create_opts = {
.type = QEMU_OPT_STRING,
.help = "Image format of the base image"
},
+ {
+ .name = BLOCK_OPT_DATA_FILE,
+ .type = QEMU_OPT_STRING,
+ .help = "File name of an external data file"
+ },
{
.name = BLOCK_OPT_ENCRYPT,
.type = QEMU_OPT_BOOL,
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 0ce18c075b..7dc59f6075 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -48,6 +48,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -69,6 +70,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -90,6 +92,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -111,6 +114,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -132,6 +136,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -153,6 +158,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -174,6 +180,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -195,6 +202,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -231,6 +239,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -304,6 +313,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -325,6 +335,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -346,6 +357,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -367,6 +379,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -388,6 +401,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -409,6 +423,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -430,6 +445,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -451,6 +467,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -487,6 +504,7 @@ Supported options:
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -568,6 +586,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -590,6 +609,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -612,6 +632,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -634,6 +655,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -656,6 +678,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -678,6 +701,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -700,6 +724,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -722,6 +747,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
@@ -761,6 +787,7 @@ Creation options for 'qcow2':
backing_fmt=<str> - Image format of the base image
cluster_size=<size> - qcow2 cluster size
compat=<str> - Compatibility level (0.10 or 1.1)
+ data_file=<str> - File name of an external data file
encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
encrypt.cipher-mode=<str> - Name of encryption cipher mode
encrypt.format=<str> - Encrypt the image, format choices: 'aes', 'luks'
--
2.20.1
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image Kevin Wolf
@ 2019-01-31 22:39 ` Nir Soffer
2019-02-19 0:18 ` Max Reitz
1 sibling, 0 replies; 46+ messages in thread
From: Nir Soffer @ 2019-01-31 22:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, QEMU Developers, Max Reitz
On Thu, Jan 31, 2019 at 8:20 PM Kevin Wolf <kwolf@redhat.com> wrote:
> Rather than requiring that the external data file node is passed
> explicitly when creating the qcow2 node, store the filename in the
> designated header extension during .bdrv_create and read it from there
> as a default during .bdrv_open.
>
If the data file is the backing file, we don't need this change.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qcow2.h | 1 +
> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/082.out | 27 +++++++++++++++
> 3 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index e2114900b4..a1e2600643 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -341,6 +341,7 @@ typedef struct BDRVQcow2State {
> * override) */
> char *image_backing_file;
> char *image_backing_format;
> + char *image_data_file;
>
> CoQueue compress_wait_queue;
> int nb_compress_threads;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6cf862e8b9..4959bf16a4 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState
> *bs, uint64_t start_offset,
> #endif
> break;
>
> + case QCOW2_EXT_MAGIC_DATA_FILE:
> + {
> + s->image_data_file = g_malloc0(ext.len + 1);
> + ret = bdrv_pread(bs->file, offset, s->image_data_file,
> ext.len);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "ERROR: Could not data file
> name");
> + return ret;
> + }
> +#ifdef DEBUG_EXT
> + printf("Qcow2: Got external data file %s\n",
> s->image_data_file);
> +#endif
> + break;
> + }
> +
> default:
> /* unknown magic - save it in case we need to rewrite the
> header */
> /* If you add a new feature, make sure to also update the fast
> @@ -1444,7 +1458,18 @@ static int coroutine_fn
> qcow2_do_open(BlockDriverState *bs, QDict *options,
> /* 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);
> + &child_file, false, &local_err);
> + if (!s->data_file) {
> + if (s->image_data_file) {
> + error_free(local_err);
> + local_err = NULL;
> + s->data_file = bdrv_open_child(s->image_data_file,
> options,
> + "data-file", bs,
> &child_file,
> + false, errp);
> + } else {
> + error_propagate(errp, local_err);
> + }
> + }
> if (!s->data_file) {
> ret = -EINVAL;
> goto fail;
> @@ -1627,6 +1652,7 @@ static int coroutine_fn
> qcow2_do_open(BlockDriverState *bs, QDict *options,
> return ret;
>
> fail:
> + g_free(s->image_data_file);
> if (has_data_file(bs)) {
> bdrv_unref_child(bs, s->data_file);
> }
> @@ -2246,6 +2272,7 @@ static void qcow2_close(BlockDriverState *bs)
> g_free(s->unknown_header_fields);
> cleanup_unknown_header_ext(bs);
>
> + g_free(s->image_data_file);
> g_free(s->image_backing_file);
> g_free(s->image_backing_format);
>
> @@ -2422,6 +2449,19 @@ int qcow2_update_header(BlockDriverState *bs)
> buflen -= ret;
> }
>
> + /* External data file header extension */
> + if (has_data_file(bs) && s->image_data_file) {
> + ret = header_ext_add(buf, QCOW2_EXT_MAGIC_DATA_FILE,
> + s->image_data_file,
> strlen(s->image_data_file),
> + buflen);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> + buf += ret;
> + buflen -= ret;
> + }
> +
> /* Full disk encryption header pointer extension */
> if (s->crypto_header.offset != 0) {
> s->crypto_header.offset = cpu_to_be64(s->crypto_header.offset);
> @@ -3166,6 +3206,7 @@ static int coroutine_fn qcow2_co_create_opts(const
> char *filename, QemuOpts *opt
> QDict *qdict;
> Visitor *v;
> BlockDriverState *bs = NULL;
> + BlockDriverState *data_bs = NULL;
> Error *local_err = NULL;
> const char *val;
> int ret;
> @@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const
> char *filename, QemuOpts *opt
> goto finish;
> }
>
> + /* Create and open an external data file (protocol layer) */
> + val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
> + if (val) {
> + ret = bdrv_create_file(val, opts, errp);
> + if (ret < 0) {
> + goto finish;
> + }
> +
> + data_bs = bdrv_open(val, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> + errp);
> + if (data_bs == NULL) {
> + ret = -EIO;
> + goto finish;
> + }
> +
> + qdict_del(qdict, BLOCK_OPT_DATA_FILE);
> + qdict_put_str(qdict, "data-file", data_bs->node_name);
> + }
> +
> /* Set 'driver' and 'node' options */
> qdict_put_str(qdict, "driver", "qcow2");
> qdict_put_str(qdict, "file", bs->node_name);
> @@ -3263,6 +3324,7 @@ static int coroutine_fn qcow2_co_create_opts(const
> char *filename, QemuOpts *opt
> finish:
> qobject_unref(qdict);
> bdrv_unref(bs);
> + bdrv_unref(data_bs);
> qapi_free_BlockdevCreateOptions(create_options);
> return ret;
> }
> @@ -4943,6 +5005,11 @@ static QemuOptsList qcow2_create_opts = {
> .type = QEMU_OPT_STRING,
> .help = "Image format of the base image"
> },
> + {
> + .name = BLOCK_OPT_DATA_FILE,
> + .type = QEMU_OPT_STRING,
> + .help = "File name of an external data file"
> + },
> {
> .name = BLOCK_OPT_ENCRYPT,
> .type = QEMU_OPT_BOOL,
> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
> index 0ce18c075b..7dc59f6075 100644
> --- a/tests/qemu-iotests/082.out
> +++ b/tests/qemu-iotests/082.out
> @@ -48,6 +48,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -69,6 +70,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -90,6 +92,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -111,6 +114,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -132,6 +136,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -153,6 +158,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -174,6 +180,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -195,6 +202,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -231,6 +239,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -304,6 +313,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -325,6 +335,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -346,6 +357,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -367,6 +379,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -388,6 +401,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -409,6 +423,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -430,6 +445,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -451,6 +467,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -487,6 +504,7 @@ Supported options:
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -568,6 +586,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -590,6 +609,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -612,6 +632,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -634,6 +655,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -656,6 +678,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -678,6 +701,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -700,6 +724,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -722,6 +747,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> @@ -761,6 +787,7 @@ Creation options for 'qcow2':
> backing_fmt=<str> - Image format of the base image
> cluster_size=<size> - qcow2 cluster size
> compat=<str> - Compatibility level (0.10 or 1.1)
> + data_file=<str> - File name of an external data file
> encrypt.cipher-alg=<str> - Name of encryption cipher algorithm
> encrypt.cipher-mode=<str> - Name of encryption cipher mode
> encrypt.format=<str> - Encrypt the image, format choices: 'aes',
> 'luks'
> --
> 2.20.1
>
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image Kevin Wolf
2019-01-31 22:39 ` Nir Soffer
@ 2019-02-19 0:18 ` Max Reitz
2019-02-19 9:04 ` Kevin Wolf
1 sibling, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-19 0:18 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4246 bytes --]
On 31.01.19 18:55, Kevin Wolf wrote:
> Rather than requiring that the external data file node is passed
> explicitly when creating the qcow2 node, store the filename in the
> designated header extension during .bdrv_create and read it from there
> as a default during .bdrv_open.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qcow2.h | 1 +
> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
> tests/qemu-iotests/082.out | 27 +++++++++++++++
> 3 files changed, 96 insertions(+), 1 deletion(-)
[...]
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6cf862e8b9..4959bf16a4 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> #endif
> break;
>
> + case QCOW2_EXT_MAGIC_DATA_FILE:
> + {
> + s->image_data_file = g_malloc0(ext.len + 1);
> + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
> + if (ret < 0) {
> + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
I think you accidentally a word.
> + return ret;
> + }
> +#ifdef DEBUG_EXT
> + printf("Qcow2: Got external data file %s\n", s->image_data_file);
> +#endif
> + break;
> + }
> +
> default:
> /* unknown magic - save it in case we need to rewrite the header */
> /* If you add a new feature, make sure to also update the fast
> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> /* 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);
> + &child_file, false, &local_err);
> + if (!s->data_file) {
> + if (s->image_data_file) {
> + error_free(local_err);
> + local_err = NULL;
This looked a bit weird to me at first because I was wondering why you
wouldn't just pass allow_none=true and then handle errors (by passing
them on). But right, we want to retry with a filename set, maybe that
makes more sense of the options.
Hm. But then again, do we really? It matches what we do with backing
files, but that does give at least me headaches from time to time. How
bad would it be to allow either passing all valid options through
@options (which would make qcow2 ignore the string in the header), or
use the filename given in the header alone?
> + s->data_file = bdrv_open_child(s->image_data_file, options,
> + "data-file", bs, &child_file,
> + false, errp);
> + } else {
> + error_propagate(errp, local_err);
> + }
> + }
> if (!s->data_file) {
> ret = -EINVAL;
> goto fail;
[...]
> @@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
> goto finish;
> }
>
> + /* Create and open an external data file (protocol layer) */
> + val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
> + if (val) {
> + ret = bdrv_create_file(val, opts, errp);
I suppose taking an existing file is saved for later?
Max
> + if (ret < 0) {
> + goto finish;
> + }
> +
> + data_bs = bdrv_open(val, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> + errp);
> + if (data_bs == NULL) {
> + ret = -EIO;
> + goto finish;
> + }
> +
> + qdict_del(qdict, BLOCK_OPT_DATA_FILE);
> + qdict_put_str(qdict, "data-file", data_bs->node_name);
> + }
> +
> /* Set 'driver' and 'node' options */
> qdict_put_str(qdict, "driver", "qcow2");
> qdict_put_str(qdict, "file", bs->node_name);
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-02-19 0:18 ` Max Reitz
@ 2019-02-19 9:04 ` Kevin Wolf
2019-02-22 14:16 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-19 9:04 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4542 bytes --]
Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
> On 31.01.19 18:55, Kevin Wolf wrote:
> > Rather than requiring that the external data file node is passed
> > explicitly when creating the qcow2 node, store the filename in the
> > designated header extension during .bdrv_create and read it from there
> > as a default during .bdrv_open.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > block/qcow2.h | 1 +
> > block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
> > tests/qemu-iotests/082.out | 27 +++++++++++++++
> > 3 files changed, 96 insertions(+), 1 deletion(-)
>
> [...]
>
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 6cf862e8b9..4959bf16a4 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> > #endif
> > break;
> >
> > + case QCOW2_EXT_MAGIC_DATA_FILE:
> > + {
> > + s->image_data_file = g_malloc0(ext.len + 1);
> > + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
> > + if (ret < 0) {
> > + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
>
> I think you accidentally a word.
>
> > + return ret;
> > + }
> > +#ifdef DEBUG_EXT
> > + printf("Qcow2: Got external data file %s\n", s->image_data_file);
> > +#endif
> > + break;
> > + }
> > +
> > default:
> > /* unknown magic - save it in case we need to rewrite the header */
> > /* If you add a new feature, make sure to also update the fast
> > @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> > /* 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);
> > + &child_file, false, &local_err);
> > + if (!s->data_file) {
> > + if (s->image_data_file) {
> > + error_free(local_err);
> > + local_err = NULL;
>
> This looked a bit weird to me at first because I was wondering why you
> wouldn't just pass allow_none=true and then handle errors (by passing
> them on). But right, we want to retry with a filename set, maybe that
> makes more sense of the options.
I think we want the normal error message for the !s->image_data_file
case. With allow_none=true, we would have to come up with a new one here
(in the else branch below).
> Hm. But then again, do we really? It matches what we do with backing
> files, but that does give at least me headaches from time to time. How
> bad would it be to allow either passing all valid options through
> @options (which would make qcow2 ignore the string in the header), or
> use the filename given in the header alone?
You mean offering only one of the two ways to configure the node?
The 'data-file' runtime option is a must so that libvirt can build the
graph node by node (and possibly use file descriptor passing one day).
But having to specify the option every time is very unfriendly for human
users, so I think allowing to store the file name in the header is a
must, too.
> > + s->data_file = bdrv_open_child(s->image_data_file, options,
> > + "data-file", bs, &child_file,
> > + false, errp);
> > + } else {
> > + error_propagate(errp, local_err);
> > + }
> > + }
> > if (!s->data_file) {
> > ret = -EINVAL;
> > goto fail;
>
> [...]
>
> > @@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
> > goto finish;
> > }
> >
> > + /* Create and open an external data file (protocol layer) */
> > + val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
> > + if (val) {
> > + ret = bdrv_create_file(val, opts, errp);
>
> I suppose taking an existing file is saved for later?
I think it's saved for the day that 'qemu-img create' is extended (or
replaced with an alternative) to provide the same functionality as QMP
blockdev-create.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-02-19 9:04 ` Kevin Wolf
@ 2019-02-22 14:16 ` Max Reitz
2019-02-22 15:35 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-22 14:16 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4983 bytes --]
On 19.02.19 10:04, Kevin Wolf wrote:
> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
>> On 31.01.19 18:55, Kevin Wolf wrote:
>>> Rather than requiring that the external data file node is passed
>>> explicitly when creating the qcow2 node, store the filename in the
>>> designated header extension during .bdrv_create and read it from there
>>> as a default during .bdrv_open.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> block/qcow2.h | 1 +
>>> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
>>> tests/qemu-iotests/082.out | 27 +++++++++++++++
>>> 3 files changed, 96 insertions(+), 1 deletion(-)
>>
>> [...]
>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 6cf862e8b9..4959bf16a4 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>> #endif
>>> break;
>>>
>>> + case QCOW2_EXT_MAGIC_DATA_FILE:
>>> + {
>>> + s->image_data_file = g_malloc0(ext.len + 1);
>>> + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
>>> + if (ret < 0) {
>>> + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
>>
>> I think you accidentally a word.
>>
>>> + return ret;
>>> + }
>>> +#ifdef DEBUG_EXT
>>> + printf("Qcow2: Got external data file %s\n", s->image_data_file);
>>> +#endif
>>> + break;
>>> + }
>>> +
>>> default:
>>> /* unknown magic - save it in case we need to rewrite the header */
>>> /* If you add a new feature, make sure to also update the fast
>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>> /* 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);
>>> + &child_file, false, &local_err);
>>> + if (!s->data_file) {
>>> + if (s->image_data_file) {
>>> + error_free(local_err);
>>> + local_err = NULL;
>>
>> This looked a bit weird to me at first because I was wondering why you
>> wouldn't just pass allow_none=true and then handle errors (by passing
>> them on). But right, we want to retry with a filename set, maybe that
>> makes more sense of the options.
>
> I think we want the normal error message for the !s->image_data_file
> case. With allow_none=true, we would have to come up with a new one here
> (in the else branch below).
>
>> Hm. But then again, do we really? It matches what we do with backing
>> files, but that does give at least me headaches from time to time. How
>> bad would it be to allow either passing all valid options through
>> @options (which would make qcow2 ignore the string in the header), or
>> use the filename given in the header alone?
>
> You mean offering only one of the two ways to configure the node?
Either just the filename from the image header, or ignore that and take
all options from the user (who'd have to give a syntactically complete
QAPI BlockdevRef object).
> The 'data-file' runtime option is a must so that libvirt can build the
> graph node by node (and possibly use file descriptor passing one day).
> But having to specify the option every time is very unfriendly for human
> users, so I think allowing to store the file name in the header is a
> must, too.
Sure. But I don't know whether we have to support taking the filename
from the image header, and the user overriding some of the node's
options (e.g. caching).
>>> + s->data_file = bdrv_open_child(s->image_data_file, options,
>>> + "data-file", bs, &child_file,
>>> + false, errp);
>>> + } else {
>>> + error_propagate(errp, local_err);
>>> + }
>>> + }
>>> if (!s->data_file) {
>>> ret = -EINVAL;
>>> goto fail;
>>
>> [...]
>>
>>> @@ -3229,6 +3270,26 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
>>> goto finish;
>>> }
>>>
>>> + /* Create and open an external data file (protocol layer) */
>>> + val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
>>> + if (val) {
>>> + ret = bdrv_create_file(val, opts, errp);
>>
>> I suppose taking an existing file is saved for later?
>
> I think it's saved for the day that 'qemu-img create' is extended (or
> replaced with an alternative) to provide the same functionality as QMP
> blockdev-create.
Ah, yes. :-)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-02-22 14:16 ` Max Reitz
@ 2019-02-22 15:35 ` Kevin Wolf
2019-02-22 15:43 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-22 15:35 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]
Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
> On 19.02.19 10:04, Kevin Wolf wrote:
> > Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
> >> On 31.01.19 18:55, Kevin Wolf wrote:
> >>> Rather than requiring that the external data file node is passed
> >>> explicitly when creating the qcow2 node, store the filename in the
> >>> designated header extension during .bdrv_create and read it from there
> >>> as a default during .bdrv_open.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>> block/qcow2.h | 1 +
> >>> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
> >>> tests/qemu-iotests/082.out | 27 +++++++++++++++
> >>> 3 files changed, 96 insertions(+), 1 deletion(-)
> >>
> >> [...]
> >>
> >>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>> index 6cf862e8b9..4959bf16a4 100644
> >>> --- a/block/qcow2.c
> >>> +++ b/block/qcow2.c
> >>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> >>> #endif
> >>> break;
> >>>
> >>> + case QCOW2_EXT_MAGIC_DATA_FILE:
> >>> + {
> >>> + s->image_data_file = g_malloc0(ext.len + 1);
> >>> + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
> >>> + if (ret < 0) {
> >>> + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
> >>
> >> I think you accidentally a word.
> >>
> >>> + return ret;
> >>> + }
> >>> +#ifdef DEBUG_EXT
> >>> + printf("Qcow2: Got external data file %s\n", s->image_data_file);
> >>> +#endif
> >>> + break;
> >>> + }
> >>> +
> >>> default:
> >>> /* unknown magic - save it in case we need to rewrite the header */
> >>> /* If you add a new feature, make sure to also update the fast
> >>> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>> /* 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);
> >>> + &child_file, false, &local_err);
> >>> + if (!s->data_file) {
> >>> + if (s->image_data_file) {
> >>> + error_free(local_err);
> >>> + local_err = NULL;
> >>
> >> This looked a bit weird to me at first because I was wondering why you
> >> wouldn't just pass allow_none=true and then handle errors (by passing
> >> them on). But right, we want to retry with a filename set, maybe that
> >> makes more sense of the options.
> >
> > I think we want the normal error message for the !s->image_data_file
> > case. With allow_none=true, we would have to come up with a new one here
> > (in the else branch below).
> >
> >> Hm. But then again, do we really? It matches what we do with backing
> >> files, but that does give at least me headaches from time to time. How
> >> bad would it be to allow either passing all valid options through
> >> @options (which would make qcow2 ignore the string in the header), or
> >> use the filename given in the header alone?
> >
> > You mean offering only one of the two ways to configure the node?
>
> Either just the filename from the image header, or ignore that and take
> all options from the user (who'd have to give a syntactically complete
> QAPI BlockdevRef object).
>
> > The 'data-file' runtime option is a must so that libvirt can build the
> > graph node by node (and possibly use file descriptor passing one day).
> > But having to specify the option every time is very unfriendly for human
> > users, so I think allowing to store the file name in the header is a
> > must, too.
>
> Sure. But I don't know whether we have to support taking the filename
> from the image header, and the user overriding some of the node's
> options (e.g. caching).
So essentially this would mean passing NULL instead of options to
bdrv_open_child() when we retry with the filename from the header.
But it's inconsistent with all other places, which comes with two
problems. It's confusing for users who are used to overriding just that
one option of a child. And do we actually spare you any headaches or do
we create new ones because we have now two different behaviours of
bdrv_open_child() callers that we must preserve in the future?
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-02-22 15:35 ` Kevin Wolf
@ 2019-02-22 15:43 ` Max Reitz
2019-02-22 16:06 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-22 15:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5298 bytes --]
On 22.02.19 16:35, Kevin Wolf wrote:
> Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
>> On 19.02.19 10:04, Kevin Wolf wrote:
>>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
>>>> On 31.01.19 18:55, Kevin Wolf wrote:
>>>>> Rather than requiring that the external data file node is passed
>>>>> explicitly when creating the qcow2 node, store the filename in the
>>>>> designated header extension during .bdrv_create and read it from there
>>>>> as a default during .bdrv_open.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>> ---
>>>>> block/qcow2.h | 1 +
>>>>> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
>>>>> tests/qemu-iotests/082.out | 27 +++++++++++++++
>>>>> 3 files changed, 96 insertions(+), 1 deletion(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>> index 6cf862e8b9..4959bf16a4 100644
>>>>> --- a/block/qcow2.c
>>>>> +++ b/block/qcow2.c
>>>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>> #endif
>>>>> break;
>>>>>
>>>>> + case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>> + {
>>>>> + s->image_data_file = g_malloc0(ext.len + 1);
>>>>> + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
>>>>> + if (ret < 0) {
>>>>> + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
>>>>
>>>> I think you accidentally a word.
>>>>
>>>>> + return ret;
>>>>> + }
>>>>> +#ifdef DEBUG_EXT
>>>>> + printf("Qcow2: Got external data file %s\n", s->image_data_file);
>>>>> +#endif
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> default:
>>>>> /* unknown magic - save it in case we need to rewrite the header */
>>>>> /* If you add a new feature, make sure to also update the fast
>>>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>>> /* 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);
>>>>> + &child_file, false, &local_err);
>>>>> + if (!s->data_file) {
>>>>> + if (s->image_data_file) {
>>>>> + error_free(local_err);
>>>>> + local_err = NULL;
>>>>
>>>> This looked a bit weird to me at first because I was wondering why you
>>>> wouldn't just pass allow_none=true and then handle errors (by passing
>>>> them on). But right, we want to retry with a filename set, maybe that
>>>> makes more sense of the options.
>>>
>>> I think we want the normal error message for the !s->image_data_file
>>> case. With allow_none=true, we would have to come up with a new one here
>>> (in the else branch below).
>>>
>>>> Hm. But then again, do we really? It matches what we do with backing
>>>> files, but that does give at least me headaches from time to time. How
>>>> bad would it be to allow either passing all valid options through
>>>> @options (which would make qcow2 ignore the string in the header), or
>>>> use the filename given in the header alone?
>>>
>>> You mean offering only one of the two ways to configure the node?
>>
>> Either just the filename from the image header, or ignore that and take
>> all options from the user (who'd have to give a syntactically complete
>> QAPI BlockdevRef object).
>>
>>> The 'data-file' runtime option is a must so that libvirt can build the
>>> graph node by node (and possibly use file descriptor passing one day).
>>> But having to specify the option every time is very unfriendly for human
>>> users, so I think allowing to store the file name in the header is a
>>> must, too.
>>
>> Sure. But I don't know whether we have to support taking the filename
>> from the image header, and the user overriding some of the node's
>> options (e.g. caching).
>
> So essentially this would mean passing NULL instead of options to
> bdrv_open_child() when we retry with the filename from the header.
>
> But it's inconsistent with all other places, which comes with two
"all other places"? Really it's just backing files, as everywhere else
there is no filename that doesn't come from the command line.
Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
that case a bit different. Although maybe it really isn't. *shrug*
> problems. It's confusing for users who are used to overriding just that
> one option of a child. And do we actually spare you any headaches or do
> we create new ones because we have now two different behaviours of
> bdrv_open_child() callers that we must preserve in the future?
It means I can act out on my pain by being angry on how .backing
behaves. That's better for my health than having to keep it in because
it's the same behavior everywhere and it's officially me who's in the wrong.
More seriously, though, yeah, it was just a random idea. It makes sense
to have the same behavior here as for .backing.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-02-22 15:43 ` Max Reitz
@ 2019-02-22 16:06 ` Kevin Wolf
2019-02-22 16:22 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-22 16:06 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5949 bytes --]
Am 22.02.2019 um 16:43 hat Max Reitz geschrieben:
> On 22.02.19 16:35, Kevin Wolf wrote:
> > Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
> >> On 19.02.19 10:04, Kevin Wolf wrote:
> >>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
> >>>> On 31.01.19 18:55, Kevin Wolf wrote:
> >>>>> Rather than requiring that the external data file node is passed
> >>>>> explicitly when creating the qcow2 node, store the filename in the
> >>>>> designated header extension during .bdrv_create and read it from there
> >>>>> as a default during .bdrv_open.
> >>>>>
> >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>>>> ---
> >>>>> block/qcow2.h | 1 +
> >>>>> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
> >>>>> tests/qemu-iotests/082.out | 27 +++++++++++++++
> >>>>> 3 files changed, 96 insertions(+), 1 deletion(-)
> >>>>
> >>>> [...]
> >>>>
> >>>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>> index 6cf862e8b9..4959bf16a4 100644
> >>>>> --- a/block/qcow2.c
> >>>>> +++ b/block/qcow2.c
> >>>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> >>>>> #endif
> >>>>> break;
> >>>>>
> >>>>> + case QCOW2_EXT_MAGIC_DATA_FILE:
> >>>>> + {
> >>>>> + s->image_data_file = g_malloc0(ext.len + 1);
> >>>>> + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
> >>>>> + if (ret < 0) {
> >>>>> + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
> >>>>
> >>>> I think you accidentally a word.
> >>>>
> >>>>> + return ret;
> >>>>> + }
> >>>>> +#ifdef DEBUG_EXT
> >>>>> + printf("Qcow2: Got external data file %s\n", s->image_data_file);
> >>>>> +#endif
> >>>>> + break;
> >>>>> + }
> >>>>> +
> >>>>> default:
> >>>>> /* unknown magic - save it in case we need to rewrite the header */
> >>>>> /* If you add a new feature, make sure to also update the fast
> >>>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> >>>>> /* 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);
> >>>>> + &child_file, false, &local_err);
> >>>>> + if (!s->data_file) {
> >>>>> + if (s->image_data_file) {
> >>>>> + error_free(local_err);
> >>>>> + local_err = NULL;
> >>>>
> >>>> This looked a bit weird to me at first because I was wondering why you
> >>>> wouldn't just pass allow_none=true and then handle errors (by passing
> >>>> them on). But right, we want to retry with a filename set, maybe that
> >>>> makes more sense of the options.
> >>>
> >>> I think we want the normal error message for the !s->image_data_file
> >>> case. With allow_none=true, we would have to come up with a new one here
> >>> (in the else branch below).
> >>>
> >>>> Hm. But then again, do we really? It matches what we do with backing
> >>>> files, but that does give at least me headaches from time to time. How
> >>>> bad would it be to allow either passing all valid options through
> >>>> @options (which would make qcow2 ignore the string in the header), or
> >>>> use the filename given in the header alone?
> >>>
> >>> You mean offering only one of the two ways to configure the node?
> >>
> >> Either just the filename from the image header, or ignore that and take
> >> all options from the user (who'd have to give a syntactically complete
> >> QAPI BlockdevRef object).
> >>
> >>> The 'data-file' runtime option is a must so that libvirt can build the
> >>> graph node by node (and possibly use file descriptor passing one day).
> >>> But having to specify the option every time is very unfriendly for human
> >>> users, so I think allowing to store the file name in the header is a
> >>> must, too.
> >>
> >> Sure. But I don't know whether we have to support taking the filename
> >> from the image header, and the user overriding some of the node's
> >> options (e.g. caching).
> >
> > So essentially this would mean passing NULL instead of options to
> > bdrv_open_child() when we retry with the filename from the header.
> >
> > But it's inconsistent with all other places, which comes with two
>
> "all other places"? Really it's just backing files, as everywhere else
> there is no filename that doesn't come from the command line.
>
> Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
> that case a bit different. Although maybe it really isn't. *shrug*
Why would it be different? At least as a user, I consider them the same.
It's true that bs->file and bs->backing come with some additional magic,
but I don't think it makes a difference for this aspect.
I'll add the third example I can think of and I'm sure you'll love this:
$ x86_64-softmmu/qemu-system-x86_64 \
-drive file=/tmp/test.vmdk,extents.0.cache.no-flush=on
So we have three examples that work this way. Can you think of any
existing counterexample?
> > problems. It's confusing for users who are used to overriding just that
> > one option of a child. And do we actually spare you any headaches or do
> > we create new ones because we have now two different behaviours of
> > bdrv_open_child() callers that we must preserve in the future?
>
> It means I can act out on my pain by being angry on how .backing
> behaves. That's better for my health than having to keep it in because
> it's the same behavior everywhere and it's officially me who's in the wrong.
You're officially wrong. ;-)
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image
2019-02-22 16:06 ` Kevin Wolf
@ 2019-02-22 16:22 ` Max Reitz
0 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2019-02-22 16:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7556 bytes --]
On 22.02.19 17:06, Kevin Wolf wrote:
> Am 22.02.2019 um 16:43 hat Max Reitz geschrieben:
>> On 22.02.19 16:35, Kevin Wolf wrote:
>>> Am 22.02.2019 um 15:16 hat Max Reitz geschrieben:
>>>> On 19.02.19 10:04, Kevin Wolf wrote:
>>>>> Am 19.02.2019 um 01:18 hat Max Reitz geschrieben:
>>>>>> On 31.01.19 18:55, Kevin Wolf wrote:
>>>>>>> Rather than requiring that the external data file node is passed
>>>>>>> explicitly when creating the qcow2 node, store the filename in the
>>>>>>> designated header extension during .bdrv_create and read it from there
>>>>>>> as a default during .bdrv_open.
>>>>>>>
>>>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>>>> ---
>>>>>>> block/qcow2.h | 1 +
>>>>>>> block/qcow2.c | 69 +++++++++++++++++++++++++++++++++++++-
>>>>>>> tests/qemu-iotests/082.out | 27 +++++++++++++++
>>>>>>> 3 files changed, 96 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>> index 6cf862e8b9..4959bf16a4 100644
>>>>>>> --- a/block/qcow2.c
>>>>>>> +++ b/block/qcow2.c
>>>>>>> @@ -398,6 +398,20 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>>>> #endif
>>>>>>> break;
>>>>>>>
>>>>>>> + case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>>>> + {
>>>>>>> + s->image_data_file = g_malloc0(ext.len + 1);
>>>>>>> + ret = bdrv_pread(bs->file, offset, s->image_data_file, ext.len);
>>>>>>> + if (ret < 0) {
>>>>>>> + error_setg_errno(errp, -ret, "ERROR: Could not data file name");
>>>>>>
>>>>>> I think you accidentally a word.
>>>>>>
>>>>>>> + return ret;
>>>>>>> + }
>>>>>>> +#ifdef DEBUG_EXT
>>>>>>> + printf("Qcow2: Got external data file %s\n", s->image_data_file);
>>>>>>> +#endif
>>>>>>> + break;
>>>>>>> + }
>>>>>>> +
>>>>>>> default:
>>>>>>> /* unknown magic - save it in case we need to rewrite the header */
>>>>>>> /* If you add a new feature, make sure to also update the fast
>>>>>>> @@ -1444,7 +1458,18 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>>>>>> /* 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);
>>>>>>> + &child_file, false, &local_err);
>>>>>>> + if (!s->data_file) {
>>>>>>> + if (s->image_data_file) {
>>>>>>> + error_free(local_err);
>>>>>>> + local_err = NULL;
>>>>>>
>>>>>> This looked a bit weird to me at first because I was wondering why you
>>>>>> wouldn't just pass allow_none=true and then handle errors (by passing
>>>>>> them on). But right, we want to retry with a filename set, maybe that
>>>>>> makes more sense of the options.
>>>>>
>>>>> I think we want the normal error message for the !s->image_data_file
>>>>> case. With allow_none=true, we would have to come up with a new one here
>>>>> (in the else branch below).
>>>>>
>>>>>> Hm. But then again, do we really? It matches what we do with backing
>>>>>> files, but that does give at least me headaches from time to time. How
>>>>>> bad would it be to allow either passing all valid options through
>>>>>> @options (which would make qcow2 ignore the string in the header), or
>>>>>> use the filename given in the header alone?
>>>>>
>>>>> You mean offering only one of the two ways to configure the node?
>>>>
>>>> Either just the filename from the image header, or ignore that and take
>>>> all options from the user (who'd have to give a syntactically complete
>>>> QAPI BlockdevRef object).
>>>>
>>>>> The 'data-file' runtime option is a must so that libvirt can build the
>>>>> graph node by node (and possibly use file descriptor passing one day).
>>>>> But having to specify the option every time is very unfriendly for human
>>>>> users, so I think allowing to store the file name in the header is a
>>>>> must, too.
>>>>
>>>> Sure. But I don't know whether we have to support taking the filename
>>>> from the image header, and the user overriding some of the node's
>>>> options (e.g. caching).
>>>
>>> So essentially this would mean passing NULL instead of options to
>>> bdrv_open_child() when we retry with the filename from the header.
>>>
>>> But it's inconsistent with all other places, which comes with two
>>
>> "all other places"? Really it's just backing files, as everywhere else
>> there is no filename that doesn't come from the command line.
>>
>> Yes, you can use -drive file=foo.qcow2,file.locking=off, but I consider
>> that case a bit different. Although maybe it really isn't. *shrug*
>
> Why would it be different? At least as a user, I consider them the same.
> It's true that bs->file and bs->backing come with some additional magic,
> but I don't think it makes a difference for this aspect.
>
> I'll add the third example I can think of and I'm sure you'll love this:
>
> $ x86_64-softmmu/qemu-system-x86_64 \
> -drive file=/tmp/test.vmdk,extents.0.cache.no-flush=on
>
> So we have three examples that work this way. Can you think of any
> existing counterexample?
Define counterexample. -drive has options for many things, so of course
I can underspecify everything. file= is basically the same as
file.filename=, so I can give .file.filename= for any node (and some
additional options, but not all that would theoretically be necessary)
and -drive will do the rest.
It's just that that's -drive magic and not driver-level magic (which
this data-file handling is, by virtue of qcow2_do_open() trying to open
the child in two different ways consecutively). Even for .backing and
the extents it's handled generically in the block layer, isn't it...?
But now I remember a real issue with .backing specifically:
$ ./qemu-img create -f qcow2 base.qcow2 64M
$ ./qemu-img create -f qcow2 -b base.qcow2 top.qcow2
$ x86_64-softmmu/qemu-system-x86_64 \
-blockdev
node-name=foo,driver=qcow2,file.driver=file,file.filename=top.qcow2,\
backing.driver=null-co
qemu-system-x86_64: -blockdev
node-name=foo,driver=qcow2,file.driver=file,file.filename=top.qcow2,backing.driver=null-co:
Could not open backing file: The only allowed filename for this driver
is 'null-co://'
Because .backing is handled by the block driver just giving the filename
to the generic block layer, which then just puts it in
backing.file.filename. It's actually pretty good that this will not
happen with your approach! In fact, that prevents a major source of
headaches I really did have with .backing. :-)
>>> problems. It's confusing for users who are used to overriding just that
>>> one option of a child. And do we actually spare you any headaches or do
>>> we create new ones because we have now two different behaviours of
>>> bdrv_open_child() callers that we must preserve in the future?
>>
>> It means I can act out on my pain by being angry on how .backing
>> behaves. That's better for my health than having to keep it in because
>> it's the same behavior everywhere and it's officially me who's in the wrong.
>
> You're officially wrong. ;-)
My poor liver. I hope it can cope with all the Paracetamol it's going
to receive.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (9 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 10/11] qcow2: Store data file name in the image Kevin Wolf
@ 2019-01-31 17:55 ` Kevin Wolf
2019-02-19 0:47 ` Max Reitz
2019-01-31 21:39 ` [Qemu-devel] [Qemu-block] [RFC PATCH 00/11] qcow2: External data files Nir Soffer
2019-02-19 0:49 ` [Qemu-devel] " Max Reitz
12 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-01-31 17:55 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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,
&child_file, false, &local_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
^ permalink raw reply related [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2 Kevin Wolf
@ 2019-02-19 0:47 ` Max Reitz
2019-02-19 9:17 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-19 0:47 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2914 bytes --]
On 31.01.19 18:55, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> 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,
> &child_file, false, &local_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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-02-19 0:47 ` Max Reitz
@ 2019-02-19 9:17 ` Kevin Wolf
2019-02-19 15:49 ` Eric Blake
2019-02-22 13:51 ` Max Reitz
0 siblings, 2 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-02-19 9:17 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3639 bytes --]
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 <kwolf@redhat.com>
> > ---
> > 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,
> > &child_file, false, &local_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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-02-19 9:17 ` Kevin Wolf
@ 2019-02-19 15:49 ` Eric Blake
2019-02-22 13:51 ` Max Reitz
1 sibling, 0 replies; 46+ messages in thread
From: Eric Blake @ 2019-02-19 15:49 UTC (permalink / raw)
To: Kevin Wolf, Max Reitz; +Cc: qemu-block, qemu-devel
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
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-02-19 9:17 ` Kevin Wolf
2019-02-19 15:49 ` Eric Blake
@ 2019-02-22 13:51 ` Max Reitz
2019-02-22 15:57 ` Kevin Wolf
1 sibling, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-22 13:51 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 4536 bytes --]
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 <kwolf@redhat.com>
>>> ---
>>> 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,
>>> &child_file, false, &local_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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-02-22 13:51 ` Max Reitz
@ 2019-02-22 15:57 ` Kevin Wolf
2019-02-22 16:13 ` Max Reitz
0 siblings, 1 reply; 46+ messages in thread
From: Kevin Wolf @ 2019-02-22 15:57 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 5521 bytes --]
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 <kwolf@redhat.com>
> >>> ---
> >>> 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,
> >>> &child_file, false, &local_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
> s->image_data_file).
I'm not exactly sure what detail in the image creation process you are
talking about.
I confirmed that this way of getting the filename into the header is
broken, and it was a known problem when I sent the series, spelt out in
the cover letter and in fact fixed in my git branch by now.
Is there anything else about the image creation process that needs
fixing?
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-02-22 15:57 ` Kevin Wolf
@ 2019-02-22 16:13 ` Max Reitz
2019-02-22 16:29 ` Kevin Wolf
0 siblings, 1 reply; 46+ messages in thread
From: Max Reitz @ 2019-02-22 16:13 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6806 bytes --]
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 <kwolf@redhat.com>
>>>>> ---
>>>>> 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,
>>>>> &child_file, false, &local_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
>> data-file option which then makes qcow2_do_open() copy it into
>> s->image_data_file).
>
> I'm not exactly sure what detail in the image creation process you are
> talking about.
Mostly the fact that when enough data-file options are provided, the
header string is overwritten with the filename taken from the node
that's been specified by those runtime options.
(Which is what the creation process uses to get the filename into the
header the first time.)
> I confirmed that this way of getting the filename into the header is
> broken, and it was a known problem when I sent the series, spelt out in
> the cover letter and in fact fixed in my git branch by now.
No need to be a bit upset. As long as we agree, it's all good -- and as
far as I remember I did acknowledge in my first response that you wrote
something about this in your cover letter.
I just wasn't sure what exactly you meant in the cover letter (to me it
read more like the filename written the first time may be different from
what the user has specified, thanks to the magic of
bdrv_refresh_filename() and other things), and that you didn't say how
you intended to fix it. So I just wanted to discuss that.
> Is there anything else about the image creation process that needs
> fixing?
It was my impression that I went already into too much detail in my
review of this series as an RFC. :-)
Also, I have to admit that I cannot guarantee a review in a fashion that
once all my requested changes are incorporated, no further versions will
be required.
In general, as I said in response to the cover letter, the series looks
good to me in principle.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2
2019-02-22 16:13 ` Max Reitz
@ 2019-02-22 16:29 ` Kevin Wolf
0 siblings, 0 replies; 46+ messages in thread
From: Kevin Wolf @ 2019-02-22 16:29 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 7613 bytes --]
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 <kwolf@redhat.com>
> >>>>> ---
> >>>>> 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,
> >>>>> &child_file, false, &local_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
> >> data-file option which then makes qcow2_do_open() copy it into
> >> s->image_data_file).
> >
> > I'm not exactly sure what detail in the image creation process you are
> > talking about.
>
> Mostly the fact that when enough data-file options are provided, the
> header string is overwritten with the filename taken from the node
> that's been specified by those runtime options.
>
> (Which is what the creation process uses to get the filename into the
> header the first time.)
Okay. That's the obvious one that is fixed with the solution I said I
was planning to use (only storing the actual image header value in
s->image_data_file).
I guess we were just talking past each other then.
> > I confirmed that this way of getting the filename into the header is
> > broken, and it was a known problem when I sent the series, spelt out in
> > the cover letter and in fact fixed in my git branch by now.
>
> No need to be a bit upset. As long as we agree, it's all good -- and as
> far as I remember I did acknowledge in my first response that you wrote
> something about this in your cover letter.
>
> I just wasn't sure what exactly you meant in the cover letter (to me it
> read more like the filename written the first time may be different from
> what the user has specified, thanks to the magic of
> bdrv_refresh_filename() and other things), and that you didn't say how
> you intended to fix it. So I just wanted to discuss that.
I'm not really upset, I was just worried that you're insisting because
there's something else wrong and I'm just too dense to understand what
it is. Seems this is actually not the case, even better.
> > Is there anything else about the image creation process that needs
> > fixing?
>
> It was my impression that I went already into too much detail in my
> review of this series as an RFC. :-)
No, this is actually very helpful. After all, I want to do as few
iterations as possible.
> Also, I have to admit that I cannot guarantee a review in a fashion that
> once all my requested changes are incorporated, no further versions will
> be required.
That's probably unavoidable in any review.
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [RFC PATCH 00/11] qcow2: External data files
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (10 preceding siblings ...)
2019-01-31 17:55 ` [Qemu-devel] [RFC PATCH 11/11] qcow2: Add data file to ImageInfoSpecificQCow2 Kevin Wolf
@ 2019-01-31 21:39 ` Nir Soffer
2019-02-19 0:49 ` [Qemu-devel] " Max Reitz
12 siblings, 0 replies; 46+ messages in thread
From: Nir Soffer @ 2019-01-31 21:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, QEMU Developers, Max Reitz
On Thu, Jan 31, 2019 at 7:57 PM Kevin Wolf <kwolf@redhat.com> wrote:
This will be very useful for new oVirt Cinder based storage. Thanks for
working on this!
I did not see any discussion about this here, but I did not follow this
list closely
lately. Do we have more info on this? a feature page describing the use
cases
and the limitations?
There are use cases where raw images are given (e.g. existing physical
> disks), but advanced features like dirty bitmaps or backing files are
> wanted that require use of a proper image format like qcow2.
>
> This series adds an incompatible feature bit to qcow2 which allows to
> use an external data file: Metadata is kept in the qcow2 file like
> usual, but guest data is written to an external file. Clusters in the
> data file are not reference counted, instead we use a flat layout where
> host cluster offset == guest cluster offset. The external data file is
> therefore readable as a raw image (though writing to it invalidates the
> associated qcow2 metadata). Features that require refcounting such as
> internal snapshots or compression are not supposed in such setups.
>
Makes sense. In oVirt we would like to use this only for raw images on
legacy
storage (no snapshot), or on LUNs managed via cinderlib, when the underlying
storage provides snapshots and thin provisioning.
If a user wants snapshot on legacy storage, they can add a qcow2 layer and
use incremental backup in this layer.
>
> There are a few reasons why this is still RFC:
>
> - The resulting code passes qemu-iotests, so we don't regress on normal
> qcow2 files, but testing with external data files is still minimal
> (converting an existing image and confirming it reads back unmodified;
> installing a guest OS and making sure it boots). We need at least some
> qemu-iotests cases.
>
> - QAPI documentation is missing
>
> - Discard isn't passed through to the data file yet
>
> - s->image_data_file isn't correct, it gets the value from the attached
> node rather than just from the image file. This means that on header
> updates we might be writing "back" a path that wasn't there before.
>
> - Probably something else I just can't remember now :-)
>
I think the most interesting questions regarding this is how we bind a raw
image
to the metadata qcow2 image, so the image cannot be used outside of a
management
system. I don't think this would safe enough without such mechanism.
I think Fam suggested in the past to encrypt the some part of the image,
e.g the first
cluster, so it is not usable without the qcow2 image. The management
system should
be able to detach a raw image from the qcow2 image using qemu-img, which
will invalidate
the metadata in the attached qcow2 image, and make the image usable outside
of the system.
If an attached image is replicated to another storage, the the original
qcow2 image is lost,
a user should be able to use qemu-img to detach the image, making it usable
outside the
system, or use qemu-img to attach is to a new qcow2 layer, allowing the
system to use the
image again.
Another question is how you attach existing raw image to qcow2 layer,
enabling incremental
backup with this volume. Do start recording changes from the point of the
attachment, so
anything not in the qcow2 layer will be available when using full backup?
Nir
>
> Kevin Wolf (11):
> qcow2: Extend spec for external data files
> qcow2: Basic definitions for external data files
> qcow2: Pass bs to qcow2_get_cluster_type()
> qcow2: Prepare qcow2_get_cluster_type() for external data file
> qcow2: Prepare count_contiguous_clusters() for external data file
> qcow2: Don't assume 0 is an invalid cluster offset
> qcow2: External file I/O
> qcow2: Add basic data-file infrastructure
> qcow2: Creating images with external data file
> qcow2: Store data file name in the image
> qcow2: Add data file to ImageInfoSpecificQCow2
>
> qapi/block-core.json | 5 +-
> docs/interop/qcow2.txt | 19 ++++-
> block/qcow2.h | 40 +++++++--
> include/block/block_int.h | 1 +
> block/qcow2-bitmap.c | 7 +-
> block/qcow2-cache.c | 6 +-
> block/qcow2-cluster.c | 144 +++++++++++++++++++-------------
> block/qcow2-refcount.c | 40 ++++++---
> block/qcow2-snapshot.c | 7 +-
> block/qcow2.c | 166 ++++++++++++++++++++++++++++++++++---
> tests/qemu-iotests/031.out | 8 +-
> tests/qemu-iotests/036.out | 4 +-
> tests/qemu-iotests/061.out | 14 ++--
> tests/qemu-iotests/082.out | 27 ++++++
> 14 files changed, 372 insertions(+), 116 deletions(-)
>
> --
> 2.20.1
>
>
>
^ permalink raw reply [flat|nested] 46+ messages in thread* Re: [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files
2019-01-31 17:55 [Qemu-devel] [RFC PATCH 00/11] qcow2: External data files Kevin Wolf
` (11 preceding siblings ...)
2019-01-31 21:39 ` [Qemu-devel] [Qemu-block] [RFC PATCH 00/11] qcow2: External data files Nir Soffer
@ 2019-02-19 0:49 ` Max Reitz
12 siblings, 0 replies; 46+ messages in thread
From: Max Reitz @ 2019-02-19 0:49 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]
On 31.01.19 18:55, Kevin Wolf wrote:
> There are use cases where raw images are given (e.g. existing physical
> disks), but advanced features like dirty bitmaps or backing files are
> wanted that require use of a proper image format like qcow2.
>
> This series adds an incompatible feature bit to qcow2 which allows to
> use an external data file: Metadata is kept in the qcow2 file like
> usual, but guest data is written to an external file. Clusters in the
> data file are not reference counted, instead we use a flat layout where
> host cluster offset == guest cluster offset. The external data file is
> therefore readable as a raw image (though writing to it invalidates the
> associated qcow2 metadata). Features that require refcounting such as
> internal snapshots or compression are not supposed in such setups.
Overall the design to me looks as simple as it could be, which means
there is equally little I could object to. I raised some technical
things, but this is an RFC anyway, so, yeah.
Looks good!
About testing... I really don't know either. I'll think about it,
maybe I come up with something.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 46+ messages in thread