| Message ID | 20200609205245.3548257-15-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/15] qemu-img: Fix doc typo for 'bitmap' subcommand | expand |
On 09.06.20 22:52, Eric Blake wrote: > From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Add class for bitmap extension and dump its fields. Further work is to > dump bitmap directory. > > Test new functionality inside 291 iotest. Unfortunately, it also breaks 291 with an external data file, which worked before. (The problems being that an external data file gives you an additional header extension, and that the bitmap directory offset changes.) I think if we want to test testing tools, we have to do that in a controlled environment where we exactly know what the image is. It looks to me now as if 291 is not such an environment. Or phrased differently, we probably shouldn’t test some testing tool in normal tests that test qemu itself. If we only test qcow2.py in normal tests, then I don’t think we have to explicitly test it at all. (Because if you test qcow2.py in a normal test, and the test breaks, it’s unclear what’s broken. So I think you might as well forego the qcow2.py test altogether, because if it breaks, that’ll probably show up in some other test case that uses it.) In this case here, I can see three things we could do: First, could just filter out the data file header extension and the bitmap_directory_offset. But I’m not sure whether that’s the best thing to do, because it might break with some other obscure IMGOPTS that I personally never use for the iotests. I think that if we want a real qcow2.py test somewhere, it should be its own test. No custom IMGOPTS allowed. So the second idea would be to move it there, and drop the qcow2.py usage from here. Or, third, maybe we actually don’t care that much about a real qcow2.py test, and really want to just *use* (as opposed to “test”) qcow2.py here. Then we should filter what we really need from its output instead of dropping what we don’t need. (We could also disable 291 for external data files, but I don’t really see why, if the only justification for this addition to it is to test qcow2.py – which 291 isn’t for.) Max > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > Message-Id: <20200606081806.23897-14-vsementsov@virtuozzo.com> > [eblake: fix iotest output] > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/qemu-iotests/291 | 4 +++ > tests/qemu-iotests/291.out | 33 +++++++++++++++++++++++ > tests/qemu-iotests/qcow2_format.py | 42 +++++++++++++++++++++++------- > 3 files changed, 70 insertions(+), 9 deletions(-) > > diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291 > index 3ca83b9cd1f7..e0cffc7cb119 100755 > --- a/tests/qemu-iotests/291 > +++ b/tests/qemu-iotests/291 > @@ -62,6 +62,8 @@ $QEMU_IO -c 'w 1M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io > $QEMU_IMG bitmap --disable -f $IMGFMT "$TEST_IMG" b1 > $QEMU_IMG bitmap --enable -f $IMGFMT "$TEST_IMG" b2 > $QEMU_IO -c 'w 2M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io > +echo "Check resulting qcow2 header extensions:" > +$PYTHON qcow2.py "$TEST_IMG" dump-header-exts > > echo > echo "=== Bitmap preservation not possible to non-qcow2 ===" > @@ -88,6 +90,8 @@ $QEMU_IMG bitmap --merge tmp -f $IMGFMT "$TEST_IMG" b0 > $QEMU_IMG bitmap --remove --image-opts \ > driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp > $QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific > +echo "Check resulting qcow2 header extensions:" > +$PYTHON qcow2.py "$TEST_IMG" dump-header-exts > > echo > echo "=== Check bitmap contents ===" > diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out > index 8c62017567e9..ccfcdc5e35ce 100644 > --- a/tests/qemu-iotests/291.out > +++ b/tests/qemu-iotests/291.out > @@ -14,6 +14,25 @@ wrote 1048576/1048576 bytes at offset 1048576 > 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > wrote 1048576/1048576 bytes at offset 2097152 > 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +Check resulting qcow2 header extensions: > +Header extension: > +magic 0xe2792aca (Backing format) > +length 5 > +data 'qcow2' > + > +Header extension: > +magic 0x6803f857 (Feature table) > +length 336 > +data <binary> > + > +Header extension: > +magic 0x23852875 (Bitmaps) > +length 24 > +nb_bitmaps 2 > +reserved32 0 > +bitmap_directory_size 0x40 > +bitmap_directory_offset 0x510000 > + > > === Bitmap preservation not possible to non-qcow2 === > > @@ -65,6 +84,20 @@ Format specific information: > granularity: 65536 > refcount bits: 16 > corrupt: false > +Check resulting qcow2 header extensions: > +Header extension: > +magic 0x6803f857 (Feature table) > +length 336 > +data <binary> > + > +Header extension: > +magic 0x23852875 (Bitmaps) > +length 24 > +nb_bitmaps 3 > +reserved32 0 > +bitmap_directory_size 0x60 > +bitmap_directory_offset 0x520000 > + > > === Check bitmap contents === >
18.06.2020 16:13, Max Reitz wrote: > On 09.06.20 22:52, Eric Blake wrote: >> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> >> Add class for bitmap extension and dump its fields. Further work is to >> dump bitmap directory. >> >> Test new functionality inside 291 iotest. > > Unfortunately, it also breaks 291 with an external data file, which > worked before. (The problems being that an external data file gives you > an additional header extension, and that the bitmap directory offset > changes.) > > I think if we want to test testing tools, we have to do that in a > controlled environment where we exactly know what the image is. It > looks to me now as if 291 is not such an environment. Or phrased > differently, we probably shouldn’t test some testing tool in normal > tests that test qemu itself. > > If we only test qcow2.py in normal tests, then I don’t think we have to > explicitly test it at all. (Because if you test qcow2.py in a normal > test, and the test breaks, it’s unclear what’s broken. So I think you > might as well forego the qcow2.py test altogether, because if it breaks, > that’ll probably show up in some other test case that uses it.) > > In this case here, I can see three things we could do: > > First, could just filter out the data file header extension and the > bitmap_directory_offset. But I’m not sure whether that’s the best thing > to do, because it might break with some other obscure IMGOPTS that I > personally never use for the iotests. > > I think that if we want a real qcow2.py test somewhere, it should be its > own test. No custom IMGOPTS allowed. So the second idea would be to > move it there, and drop the qcow2.py usage from here. > > Or, third, maybe we actually don’t care that much about a real qcow2.py > test, and really want to just *use* (as opposed to “test”) qcow2.py > here. Then we should filter what we really need from its output instead > of dropping what we don’t need. > > > (We could also disable 291 for external data files, but I don’t really > see why, if the only justification for this addition to it is to test > qcow2.py – which 291 isn’t for.) > I see your point, agree that the most correct thing is to drop qcow2.py testing from 291, reverting test chunks from this commit. I can send a patch. I do think, that we'll need some testing for qcow2.py, as we are going to improve it a lot, to be used as separate debugging tool. And we just need a separate iotest for it. Andrey, could you please introduce one, as part of your series?
On 18.06.20 15:28, Vladimir Sementsov-Ogievskiy wrote: > 18.06.2020 16:13, Max Reitz wrote: >> On 09.06.20 22:52, Eric Blake wrote: >>> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> >>> Add class for bitmap extension and dump its fields. Further work is to >>> dump bitmap directory. >>> >>> Test new functionality inside 291 iotest. >> >> Unfortunately, it also breaks 291 with an external data file, which >> worked before. (The problems being that an external data file gives you >> an additional header extension, and that the bitmap directory offset >> changes.) >> >> I think if we want to test testing tools, we have to do that in a >> controlled environment where we exactly know what the image is. It >> looks to me now as if 291 is not such an environment. Or phrased >> differently, we probably shouldn’t test some testing tool in normal >> tests that test qemu itself. >> >> If we only test qcow2.py in normal tests, then I don’t think we have to >> explicitly test it at all. (Because if you test qcow2.py in a normal >> test, and the test breaks, it’s unclear what’s broken. So I think you >> might as well forego the qcow2.py test altogether, because if it breaks, >> that’ll probably show up in some other test case that uses it.) >> >> In this case here, I can see three things we could do: >> >> First, could just filter out the data file header extension and the >> bitmap_directory_offset. But I’m not sure whether that’s the best thing >> to do, because it might break with some other obscure IMGOPTS that I >> personally never use for the iotests. >> >> I think that if we want a real qcow2.py test somewhere, it should be its >> own test. No custom IMGOPTS allowed. So the second idea would be to >> move it there, and drop the qcow2.py usage from here. >> >> Or, third, maybe we actually don’t care that much about a real qcow2.py >> test, and really want to just *use* (as opposed to “test”) qcow2.py >> here. Then we should filter what we really need from its output instead >> of dropping what we don’t need. >> >> >> (We could also disable 291 for external data files, but I don’t really >> see why, if the only justification for this addition to it is to test >> qcow2.py – which 291 isn’t for.) >> > I see your point, agree that the most correct thing is to drop qcow2.py > testing from 291, reverting test chunks from this commit. I can send a > patch. OK, thanks! > I do think, that we'll need some testing for qcow2.py, as we are going > to improve it a lot, to be used as separate debugging tool. And we just > need a separate iotest for it. Great. Max
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291 index 3ca83b9cd1f7..e0cffc7cb119 100755 --- a/tests/qemu-iotests/291 +++ b/tests/qemu-iotests/291 @@ -62,6 +62,8 @@ $QEMU_IO -c 'w 1M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io $QEMU_IMG bitmap --disable -f $IMGFMT "$TEST_IMG" b1 $QEMU_IMG bitmap --enable -f $IMGFMT "$TEST_IMG" b2 $QEMU_IO -c 'w 2M 1M' -f $IMGFMT "$TEST_IMG" | _filter_qemu_io +echo "Check resulting qcow2 header extensions:" +$PYTHON qcow2.py "$TEST_IMG" dump-header-exts echo echo "=== Bitmap preservation not possible to non-qcow2 ===" @@ -88,6 +90,8 @@ $QEMU_IMG bitmap --merge tmp -f $IMGFMT "$TEST_IMG" b0 $QEMU_IMG bitmap --remove --image-opts \ driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp $QEMU_IMG info "$TEST_IMG" | _filter_img_info --format-specific +echo "Check resulting qcow2 header extensions:" +$PYTHON qcow2.py "$TEST_IMG" dump-header-exts echo echo "=== Check bitmap contents ===" diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out index 8c62017567e9..ccfcdc5e35ce 100644 --- a/tests/qemu-iotests/291.out +++ b/tests/qemu-iotests/291.out @@ -14,6 +14,25 @@ wrote 1048576/1048576 bytes at offset 1048576 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) wrote 1048576/1048576 bytes at offset 2097152 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Check resulting qcow2 header extensions: +Header extension: +magic 0xe2792aca (Backing format) +length 5 +data 'qcow2' + +Header extension: +magic 0x6803f857 (Feature table) +length 336 +data <binary> + +Header extension: +magic 0x23852875 (Bitmaps) +length 24 +nb_bitmaps 2 +reserved32 0 +bitmap_directory_size 0x40 +bitmap_directory_offset 0x510000 + === Bitmap preservation not possible to non-qcow2 === @@ -65,6 +84,20 @@ Format specific information: granularity: 65536 refcount bits: 16 corrupt: false +Check resulting qcow2 header extensions: +Header extension: +magic 0x6803f857 (Feature table) +length 336 +data <binary> + +Header extension: +magic 0x23852875 (Bitmaps) +length 24 +nb_bitmaps 3 +reserved32 0 +bitmap_directory_size 0x60 +bitmap_directory_offset 0x520000 + === Check bitmap contents === diff --git a/tests/qemu-iotests/qcow2_format.py b/tests/qemu-iotests/qcow2_format.py index 40b5bf467b24..0f65fd161d5b 100644 --- a/tests/qemu-iotests/qcow2_format.py +++ b/tests/qemu-iotests/qcow2_format.py @@ -103,6 +103,19 @@ class Qcow2Struct(metaclass=Qcow2StructMeta): print('{:<25} {}'.format(f[2], value_str)) +class Qcow2BitmapExt(Qcow2Struct): + + fields = ( + ('u32', '{}', 'nb_bitmaps'), + ('u32', '{}', 'reserved32'), + ('u64', '{:#x}', 'bitmap_directory_size'), + ('u64', '{:#x}', 'bitmap_directory_offset') + ) + + +QCOW2_EXT_MAGIC_BITMAPS = 0x23852875 + + class QcowHeaderExtension(Qcow2Struct): class Magic(Enum): @@ -110,7 +123,7 @@ class QcowHeaderExtension(Qcow2Struct): 0xe2792aca: 'Backing format', 0x6803f857: 'Feature table', 0x0537be77: 'Crypto header', - 0x23852875: 'Bitmaps', + QCOW2_EXT_MAGIC_BITMAPS: 'Bitmaps', 0x44415441: 'Data file' } @@ -130,8 +143,11 @@ class QcowHeaderExtension(Qcow2Struct): This should be somehow refactored and functionality should be moved to superclass (to allow creation of any qcow2 struct), but then, fields of variable length (data here) should be supported in base class - somehow. So, it's a TODO. We'll see how to properly refactor this when - we have more qcow2 structures. + somehow. Note also, that we probably want to parse different + extensions. Should they be subclasses of this class, or how to do it + better? Should it be something like QAPI union with discriminator field + (magic here). So, it's a TODO. We'll see how to properly refactor this + when we have more qcow2 structures. """ if fd is None: assert all(v is not None for v in (magic, length, data)) @@ -148,15 +164,23 @@ class QcowHeaderExtension(Qcow2Struct): self.data = fd.read(padded) assert self.data is not None - def dump(self): - data = self.data[:self.length] - if all(c in string.printable.encode('ascii') for c in data): - data = f"'{ data.decode('ascii') }'" + if self.magic == QCOW2_EXT_MAGIC_BITMAPS: + self.obj = Qcow2BitmapExt(data=self.data) else: - data = '<binary>' + self.obj = None + def dump(self): super().dump() - print(f'{"data":<25} {data}') + + if self.obj is None: + data = self.data[:self.length] + if all(c in string.printable.encode('ascii') for c in data): + data = f"'{ data.decode('ascii') }'" + else: + data = '<binary>' + print(f'{"data":<25} {data}') + else: + self.obj.dump() @classmethod def create(cls, magic, data):