block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/244 | 13 +++++++++++++ tests/qemu-iotests/244.out | 14 ++++++++++++++ 3 files changed, 66 insertions(+)
Although we cannot create these images with qemu-img it is still
possible to do it using an external tool. QEMU should refuse to open
them until the data-file-raw bit is cleared with 'qemu-img check'.
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/244 | 13 +++++++++++++
tests/qemu-iotests/244.out | 14 ++++++++++++++
3 files changed, 66 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index b524b0c53f..fcc3810391 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -588,16 +588,49 @@ static void qcow2_add_check_result(BdrvCheckResult *out,
}
}
+static int qcow2_check_header(BlockDriverState *bs, BdrvCheckResult *result,
+ BdrvCheckMode fix)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int ret;
+
+ if (bs->backing && data_file_is_raw(bs)) {
+ fprintf(stderr, "%s header: data-file-raw cannot be set "
+ "when there is a backing file.\n",
+ fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR in");
+ if (fix & BDRV_FIX_ERRORS) {
+ s->autoclear_features &= ~QCOW2_AUTOCLEAR_DATA_FILE_RAW;
+ ret = qcow2_update_header(bs);
+ if (ret < 0) {
+ result->check_errors++;
+ return ret;
+ }
+ result->corruptions_fixed++;
+ } else {
+ result->corruptions++;
+ }
+ }
+
+ return 0;
+}
+
static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
BdrvCheckResult *result,
BdrvCheckMode fix)
{
+ BdrvCheckResult header_res = {};
BdrvCheckResult snapshot_res = {};
BdrvCheckResult refcount_res = {};
int ret;
memset(result, 0, sizeof(*result));
+ ret = qcow2_check_header(bs, &header_res, fix);
+ qcow2_add_check_result(result, &header_res, false);
+ if (ret < 0) {
+ return ret;
+ }
+
ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
if (ret < 0) {
qcow2_add_check_result(result, &snapshot_res, false);
@@ -1604,6 +1637,12 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* read the backing file name */
if (header.backing_file_offset != 0) {
+ if (data_file_is_raw(bs) && (!(flags & BDRV_O_CHECK))) {
+ error_setg(errp, "data-file-raw cannot be set when "
+ "there is a backing file");
+ ret = -EINVAL;
+ goto fail;
+ }
len = header.backing_file_size;
if (len > MIN(1023, s->cluster_size - header.backing_file_offset) ||
len >= sizeof(bs->backing_file)) {
diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
index 2ec1815e6f..159941acd4 100755
--- a/tests/qemu-iotests/244
+++ b/tests/qemu-iotests/244
@@ -211,6 +211,19 @@ $QEMU_IMG amend -f $IMGFMT -o "data_file=blkdebug::$TEST_IMG.data" "$TEST_IMG"
$QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n -C "$TEST_IMG.src" "$TEST_IMG"
$QEMU_IMG compare -f $IMGFMT -F $IMGFMT "$TEST_IMG.src" "$TEST_IMG"
+echo
+echo "=== Using and repairing an image with backing file and the data_file_raw bit ==="
+echo
+
+# Create an image with a backing file and an external data file
+TEST_IMG_FILE="$TEST_IMG.base" _make_test_img 1M
+_make_test_img -o "data_file=$TEST_IMG.data" -b "$TEST_IMG.base"
+# Set 'data_file_raw' directly on the header (qemu-img amend won't let us)
+poke_file "$TEST_IMG" 95 "\x02"
+# Trying to open the image should produce an error
+$QEMU_IMG info "$TEST_IMG" 2>&1 | _filter_testdir
+_check_test_img -r all
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index 56329deb4b..cab367dfb5 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -128,4 +128,18 @@ Offset Length Mapped to File
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 data_file=TEST_DIR/t.IMGFMT.data
Images are identical.
Images are identical.
+
+=== Using and repairing an image with backing file and the data_file_raw bit ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base data_file=TEST_DIR/t.IMGFMT.data
+qemu-img: Could not open 'TEST_DIR/t.qcow2': data-file-raw cannot be set when there is a backing file
+Repairing header: data-file-raw cannot be set when there is a backing file.
+The following inconsistencies were found and repaired:
+
+ 0 leaked clusters
+ 1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
*** done
--
2.20.1
ping On Wed 15 Apr 2020 09:02:07 PM CEST, Alberto Garcia wrote: > Although we cannot create these images with qemu-img it is still > possible to do it using an external tool. QEMU should refuse to open > them until the data-file-raw bit is cleared with 'qemu-img check'. > > Signed-off-by: Alberto Garcia <berto@igalia.com>
On 15.04.20 21:02, Alberto Garcia wrote:
> Although we cannot create these images with qemu-img it is still
> possible to do it using an external tool. QEMU should refuse to open
> them until the data-file-raw bit is cleared with 'qemu-img check'.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/244 | 13 +++++++++++++
> tests/qemu-iotests/244.out | 14 ++++++++++++++
> 3 files changed, 66 insertions(+)
Sorry for the long delay. :/
The patch itself looks good, but I’m not sure whether it is extensive
enough. Let me just jump straight to the problem:
$ ./qemu-img create -f qcow2 \
-o data_file=foo.qcow2.raw,data_file_raw=on \
foo.qcow2 64M
(Create some file empty foo.qcow2 with external data file that’s raw)
$ ./qemu-img create -f qcow2 backing.qcow2 64M
$ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2
(Create some file filled with 42s)
$ ./qemu-img compare foo.qcow2 foo.qcow2.raw
Images are identical.
(As expected, foo.qcow2 is identical to its raw data file)
$ ./qemu-img compare --image-opts \
file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \
file.filename=foo.qcow2.raw
Content mismatch at offset 0!
(Oops.)
So when the user manually gives a backing file without one having been
given by the image file, we run into the same problem. Now I’m not
quite sure what the problem is here. We could make this patch more
extensive and also forbid this case.
But I think there actually shouldn’t be a problem. The qcow2 driver
shouldn’t fall back to a backing file for raw external data files. But
how exactly should that be implemented? I think the correct way would
be to preallocate all metadata whenever data_file_raw=on – the qcow2
spec doesn’t say to ignore the metadata with data_file_raw=on, it just
says that the data read from the qcow2 file must match that read from
the external data file.
(I seem to remember I proposed this before, but I don’t know exactly...)
(In contrast, I don’t think it would be correct to just treat
unallocated clusters as zero whenever data_file_raw=on.)
What do you think? Should we force preallocation with data_file_raw=on,
and then just take this patch, even though it still lets users give
backing files to a qcow2 file at runtime without error? (Except the
backing file wouldn’t have an effect, then.)
On Wed 03 Jun 2020 03:53:03 PM CEST, Max Reitz wrote: > Sorry for the long delay. :/ And sorry for my long delay as well. > The patch itself looks good, but I’m not sure whether it is extensive > enough. Let me just jump straight to the problem: > > $ ./qemu-img create -f qcow2 \ > -o data_file=foo.qcow2.raw,data_file_raw=on \ > foo.qcow2 64M > (Create some file empty foo.qcow2 with external data file that’s raw) > > $ ./qemu-img create -f qcow2 backing.qcow2 64M > $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2 > (Create some file filled with 42s) > > $ ./qemu-img compare foo.qcow2 foo.qcow2.raw > Images are identical. > (As expected, foo.qcow2 is identical to its raw data file) > > $ ./qemu-img compare --image-opts \ > file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \ > file.filename=foo.qcow2.raw > Content mismatch at offset 0! > (Oops.) If two images have the same contents but then you compare them changing the backing file of one of them you can also get a content mismatch. How is this different? Regardless of how we should ideally handle bs->backing and data-file-raw (and yes I agree that it would be nice that QEMU would say "you cannot have a backing file and an external raw file" in all cases) I think that if the problem is that the user can override the backing file name and get different contents, then that's not a problem that we should be worried about. Berto
On 18.06.20 14:03, Alberto Garcia wrote: > On Wed 03 Jun 2020 03:53:03 PM CEST, Max Reitz wrote: >> Sorry for the long delay. :/ > > And sorry for my long delay as well. > >> The patch itself looks good, but I’m not sure whether it is extensive >> enough. Let me just jump straight to the problem: >> >> $ ./qemu-img create -f qcow2 \ >> -o data_file=foo.qcow2.raw,data_file_raw=on \ >> foo.qcow2 64M >> (Create some file empty foo.qcow2 with external data file that’s raw) >> >> $ ./qemu-img create -f qcow2 backing.qcow2 64M >> $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2 >> (Create some file filled with 42s) >> >> $ ./qemu-img compare foo.qcow2 foo.qcow2.raw >> Images are identical. >> (As expected, foo.qcow2 is identical to its raw data file) >> >> $ ./qemu-img compare --image-opts \ >> file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \ >> file.filename=foo.qcow2.raw >> Content mismatch at offset 0! >> (Oops.) > > If two images have the same contents but then you compare them changing > the backing file of one of them you can also get a content mismatch. How > is this different? It’s different in that files with data-file-raw can’t have backing files at all. So maybe users shouldn’t be allowed to give them backing files at runtime either. Or at least, if we have data-file-raw, *all* data visible on such an image should be taken from the raw data file, never from any backing file. > Regardless of how we should ideally handle bs->backing and data-file-raw > (and yes I agree that it would be nice that QEMU would say "you cannot > have a backing file and an external raw file" in all cases) I think that > if the problem is that the user can override the backing file name and > get different contents, then that's not a problem that we should be > worried about. Well, not really be worried about, but I do think it’s indicative of some problem, though I’m not sure whether the problem is error reporting. I think it’s rather the fact that data-file-raw should imply metadata preallocation. With preallocation, we’d ensure that we always take all data from the raw data file. So we’d always ignore any potential backing file. (It makes sense to guard users against giving images with data-file-raw a backing file, and to consider such images corrupt, as done by this patch. But if users can force a backing file at runtime, I think showing its contents is another bug.) Max
On Fri 19 Jun 2020 09:57:27 AM CEST, Max Reitz wrote: >> If two images have the same contents but then you compare them >> changing the backing file of one of them you can also get a content >> mismatch. How is this different? > > It’s different in that files with data-file-raw can’t have backing > files at all. So maybe users shouldn’t be allowed to give them > backing files at runtime either. I understand that. Ideally it should be forbidden. Perhaps that could be fixed by turning drv->supports_backing into a function. My point however is that forcing a different backing file is something that is going to cause breakage unless the user really knows that they're doing. And we don't generally forbid that, we just let the user take responsibility. So I'm not too worried about this case. > Or at least, if we have data-file-raw, *all* data visible on such an > image should be taken from the raw data file, never from any backing > file. It should be easy to handle in qcow2_co_preadv_part() and qcow2_co_copy_range_from(), but it feels hackish and error prone. > With preallocation, we’d ensure that we always take all data from the > raw data file. So we’d always ignore any potential backing file. Preallocation has its problems (and we would also have to handle it differently if there are subclusters, but I think that should be easy). But I don't have a strong opinion. Berto
Am 03.06.2020 um 15:53 hat Max Reitz geschrieben: > On 15.04.20 21:02, Alberto Garcia wrote: > > Although we cannot create these images with qemu-img it is still > > possible to do it using an external tool. QEMU should refuse to open > > them until the data-file-raw bit is cleared with 'qemu-img check'. > > > > Signed-off-by: Alberto Garcia <berto@igalia.com> > > --- > > block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ > > tests/qemu-iotests/244 | 13 +++++++++++++ > > tests/qemu-iotests/244.out | 14 ++++++++++++++ > > 3 files changed, 66 insertions(+) > > Sorry for the long delay. :/ > > The patch itself looks good, but I’m not sure whether it is extensive > enough. Let me just jump straight to the problem: > > $ ./qemu-img create -f qcow2 \ > -o data_file=foo.qcow2.raw,data_file_raw=on \ > foo.qcow2 64M > (Create some file empty foo.qcow2 with external data file that’s raw) > > $ ./qemu-img create -f qcow2 backing.qcow2 64M > $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2 > (Create some file filled with 42s) > > $ ./qemu-img compare foo.qcow2 foo.qcow2.raw > Images are identical. > (As expected, foo.qcow2 is identical to its raw data file) > > $ ./qemu-img compare --image-opts \ > file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \ > file.filename=foo.qcow2.raw > Content mismatch at offset 0! > (Oops.) > > So when the user manually gives a backing file without one having been > given by the image file, we run into the same problem. Now I’m not > quite sure what the problem is here. We could make this patch more > extensive and also forbid this case. I guess what we should really be checking is that bs->backing is NULL after the node is fully opened. The challenging part is that the backing child isn't managed by the block driver, but by the generic block layer, and .brv_open() comes first. So we don't really have a place to check this. (And there is also the case that the image is originally opened with BDRV_O_NO_BACKING and the later bdrv_open_backing_file().) > But I think there actually shouldn’t be a problem. The qcow2 driver > shouldn’t fall back to a backing file for raw external data files. But > how exactly should that be implemented? I think the correct way would > be to preallocate all metadata whenever data_file_raw=on – the qcow2 > spec doesn’t say to ignore the metadata with data_file_raw=on, it just > says that the data read from the qcow2 file must match that read from > the external data file. > (I seem to remember I proposed this before, but I don’t know exactly...) I don't find preallocation convincing, mostly for two reasons. First is, old images or images created by another program could miss the preallocation, but we still shouldn't access the backing file. The other one is that discard breaks preallocation, so we would also have to make sure to have a special case in every operation that could end up discarding clusters (and to add it to every future operation we might add). It just sounds very brittle. > (In contrast, I don’t think it would be correct to just treat > unallocated clusters as zero whenever data_file_raw=on.) > > What do you think? Should we force preallocation with data_file_raw=on, > and then just take this patch, even though it still lets users give > backing files to a qcow2 file at runtime without error? (Except the > backing file wouldn’t have an effect, then.) Honestly, maybe passing a backing file at runtime to an image that doesn't logically have one is just a case of "then don't do that". Kevin
On 05.06.20 13:14, Kevin Wolf wrote: > Am 03.06.2020 um 15:53 hat Max Reitz geschrieben: >> On 15.04.20 21:02, Alberto Garcia wrote: >>> Although we cannot create these images with qemu-img it is still >>> possible to do it using an external tool. QEMU should refuse to open >>> them until the data-file-raw bit is cleared with 'qemu-img check'. >>> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>> --- >>> block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ >>> tests/qemu-iotests/244 | 13 +++++++++++++ >>> tests/qemu-iotests/244.out | 14 ++++++++++++++ >>> 3 files changed, 66 insertions(+) >> >> Sorry for the long delay. :/ >> >> The patch itself looks good, but I’m not sure whether it is extensive >> enough. Let me just jump straight to the problem: >> >> $ ./qemu-img create -f qcow2 \ >> -o data_file=foo.qcow2.raw,data_file_raw=on \ >> foo.qcow2 64M >> (Create some file empty foo.qcow2 with external data file that’s raw) >> >> $ ./qemu-img create -f qcow2 backing.qcow2 64M >> $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2 >> (Create some file filled with 42s) >> >> $ ./qemu-img compare foo.qcow2 foo.qcow2.raw >> Images are identical. >> (As expected, foo.qcow2 is identical to its raw data file) >> >> $ ./qemu-img compare --image-opts \ >> file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \ >> file.filename=foo.qcow2.raw >> Content mismatch at offset 0! >> (Oops.) >> >> So when the user manually gives a backing file without one having been >> given by the image file, we run into the same problem. Now I’m not >> quite sure what the problem is here. We could make this patch more >> extensive and also forbid this case. > > I guess what we should really be checking is that bs->backing is NULL > after the node is fully opened. The challenging part is that the backing > child isn't managed by the block driver, but by the generic block layer, > and .brv_open() comes first. So we don't really have a place to check > this. (And there is also the case that the image is originally opened > with BDRV_O_NO_BACKING and the later bdrv_open_backing_file().) > >> But I think there actually shouldn’t be a problem. The qcow2 driver >> shouldn’t fall back to a backing file for raw external data files. But >> how exactly should that be implemented? I think the correct way would >> be to preallocate all metadata whenever data_file_raw=on – the qcow2 >> spec doesn’t say to ignore the metadata with data_file_raw=on, it just >> says that the data read from the qcow2 file must match that read from >> the external data file. >> (I seem to remember I proposed this before, but I don’t know exactly...) > > I don't find preallocation convincing, mostly for two reasons. > > First is, old images or images created by another program could miss the > preallocation, but we still shouldn't access the backing file. I’d take this patch anyway (because its motivation is just that other programs might produce invalid images), and then not worry about the case where we get an image produced by such another program (including older versions of qemu) for which the user overrides the backing file at runtime. > The other one is that discard breaks preallocation, The preallocation is about ensuring that there are no fall-through-to-backing holes in the image. Discarding doesn’t change that. > so we would also > have to make sure to have a special case in every operation that could > end up discarding clusters (and to add it to every future operation we > might add). > > It just sounds very brittle. > >> (In contrast, I don’t think it would be correct to just treat >> unallocated clusters as zero whenever data_file_raw=on.) >> >> What do you think? Should we force preallocation with data_file_raw=on, >> and then just take this patch, even though it still lets users give >> backing files to a qcow2 file at runtime without error? (Except the >> backing file wouldn’t have an effect, then.) > > Honestly, maybe passing a backing file at runtime to an image that > doesn't logically have one is just a case of "then don't do that". Perhaps. But seeing I wondered whether I didn’t already propose this at some point, there is another reason for preallocation: https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html All in all, I think data_file_raw should be interpretable as “You don’t have to look at any metadata to know which data to read or write”. (Maybe I’m wrong about that.) Without any preallocation of metadata structure, it looks to me like we break that promise. (Yes, we could also force-zero the external data file during creation, and blame users who put a backing file on images that don’t have one – both of which are not unreasonable! But we could also just preallocate the metadata.) Max
Am 05.06.2020 um 14:11 hat Max Reitz geschrieben: > On 05.06.20 13:14, Kevin Wolf wrote: > > Am 03.06.2020 um 15:53 hat Max Reitz geschrieben: > >> On 15.04.20 21:02, Alberto Garcia wrote: > >>> Although we cannot create these images with qemu-img it is still > >>> possible to do it using an external tool. QEMU should refuse to open > >>> them until the data-file-raw bit is cleared with 'qemu-img check'. > >>> > >>> Signed-off-by: Alberto Garcia <berto@igalia.com> > >>> --- > >>> block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ > >>> tests/qemu-iotests/244 | 13 +++++++++++++ > >>> tests/qemu-iotests/244.out | 14 ++++++++++++++ > >>> 3 files changed, 66 insertions(+) > >> > >> Sorry for the long delay. :/ > >> > >> The patch itself looks good, but I’m not sure whether it is extensive > >> enough. Let me just jump straight to the problem: > >> > >> $ ./qemu-img create -f qcow2 \ > >> -o data_file=foo.qcow2.raw,data_file_raw=on \ > >> foo.qcow2 64M > >> (Create some file empty foo.qcow2 with external data file that’s raw) > >> > >> $ ./qemu-img create -f qcow2 backing.qcow2 64M > >> $ ./qemu-io -c 'write -P 42 0 64M' backing.qcow2 > >> (Create some file filled with 42s) > >> > >> $ ./qemu-img compare foo.qcow2 foo.qcow2.raw > >> Images are identical. > >> (As expected, foo.qcow2 is identical to its raw data file) > >> > >> $ ./qemu-img compare --image-opts \ > >> file.filename=foo.qcow2,backing.file.filename=backing.qcow2 \ > >> file.filename=foo.qcow2.raw > >> Content mismatch at offset 0! > >> (Oops.) > >> > >> So when the user manually gives a backing file without one having been > >> given by the image file, we run into the same problem. Now I’m not > >> quite sure what the problem is here. We could make this patch more > >> extensive and also forbid this case. > > > > I guess what we should really be checking is that bs->backing is NULL > > after the node is fully opened. The challenging part is that the backing > > child isn't managed by the block driver, but by the generic block layer, > > and .brv_open() comes first. So we don't really have a place to check > > this. (And there is also the case that the image is originally opened > > with BDRV_O_NO_BACKING and the later bdrv_open_backing_file().) > > > >> But I think there actually shouldn’t be a problem. The qcow2 driver > >> shouldn’t fall back to a backing file for raw external data files. But > >> how exactly should that be implemented? I think the correct way would > >> be to preallocate all metadata whenever data_file_raw=on – the qcow2 > >> spec doesn’t say to ignore the metadata with data_file_raw=on, it just > >> says that the data read from the qcow2 file must match that read from > >> the external data file. > >> (I seem to remember I proposed this before, but I don’t know exactly...) > > > > I don't find preallocation convincing, mostly for two reasons. > > > > First is, old images or images created by another program could miss the > > preallocation, but we still shouldn't access the backing file. > > I’d take this patch anyway (because its motivation is just that other > programs might produce invalid images), and then not worry about the > case where we get an image produced by such another program (including > older versions of qemu) for which the user overrides the backing file at > runtime. > > > The other one is that discard breaks preallocation, > > The preallocation is about ensuring that there are no > fall-through-to-backing holes in the image. Discarding doesn’t change that. > > > so we would also > > have to make sure to have a special case in every operation that could > > end up discarding clusters (and to add it to every future operation we > > might add). > > > > It just sounds very brittle. > > > >> (In contrast, I don’t think it would be correct to just treat > >> unallocated clusters as zero whenever data_file_raw=on.) > >> > >> What do you think? Should we force preallocation with data_file_raw=on, > >> and then just take this patch, even though it still lets users give > >> backing files to a qcow2 file at runtime without error? (Except the > >> backing file wouldn’t have an effect, then.) > > > > Honestly, maybe passing a backing file at runtime to an image that > > doesn't logically have one is just a case of "then don't do that". > > Perhaps. > > But seeing I wondered whether I didn’t already propose this at some > point, there is another reason for preallocation: > > https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html > https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html > > All in all, I think data_file_raw should be interpretable as “You don’t > have to look at any metadata to know which data to read or write”. > (Maybe I’m wrong about that.) > Without any preallocation of metadata structure, it looks to me like we > break that promise. > > (Yes, we could also force-zero the external data file during creation, > and blame users who put a backing file on images that don’t have one – > both of which are not unreasonable! But we could also just preallocate > the metadata.) Makes sense. I'm not against preallocation during image creation. I just think it shouldn't play a role in deciding whether an image is valid or not. Kevin
On 05.06.20 15:00, Kevin Wolf wrote: [...] > Makes sense. I'm not against preallocation during image creation. I just > think it shouldn't play a role in deciding whether an image is valid or > not. Oh, no. I wouldn’t consider it corrupted just because some clusters are not allocated. I’d just say any program that handles qcow2 files is responsible to ensure that images with the data-file-raw flag actually fulfill the flag’s promise. So if a cluster isn’t allocated in qcow2, it must read as zeroes in the data file (because the spec disallows backing files with data-file-raw anyway[1]). It’s just my impression that qemu currently doesn’t always ensure this, and the easiest way to do so would be to enforce metadata preallocation for such images. Max [1] Such images are indeed corrupt, hence this patch here from Berto.
On 4/15/20 2:02 PM, Alberto Garcia wrote: > Although we cannot create these images with qemu-img it is still > possible to do it using an external tool. QEMU should refuse to open > them until the data-file-raw bit is cleared with 'qemu-img check'. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2.c | 39 ++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/244 | 13 +++++++++++++ > tests/qemu-iotests/244.out | 14 ++++++++++++++ > 3 files changed, 66 insertions(+) > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.