Message ID | 57782138d21e277dddee0fd348a8ba437411bd64.1429875134.git.berto@igalia.com |
---|---|
State | New |
Headers | show |
On 24.04.2015 13:40, Alberto Garcia wrote: > This patch updates test_stream_partial() to test that the block-stream > operation never copies data from the image specified as base. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > tests/qemu-iotests/030 | 13 +++++++++++++ > 1 file changed, 13 insertions(+) I think it would be better to add this as an own test case. But I won't oppose adding it as a special case of test_stream_partial either, because strictly speaking, streaming nothing is a special case of streaming partially. > diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 > index 6e6cb5a..a395a03 100755 > --- a/tests/qemu-iotests/030 > +++ b/tests/qemu-iotests/030 > @@ -94,6 +94,19 @@ class TestSingleDrive(iotests.QMPTestCase): > def test_stream_partial(self): > self.assert_no_active_block_jobs() > > + # This is a no-op: no data should ever be copied from the base image > + result = self.vm.qmp('block-stream', device='drive0', base=mid_img) > + self.assert_qmp(result, 'return', {}) > + > + self.wait_until_completed() > + > + self.assert_no_active_block_jobs() > + > + self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img), > + qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img), > + 'image file map matches backing file after a no-op') Well, you haven't really proven anything by doing this, except that the mid_img hasn't been streamed completely into test_img... What you want to prove is that test_img is empty after streaming, right? This can be done by either comparing against an empty image specifically created for this single test, or by comparing the qemu_io map output against "[ 0] 2048/ 2048 sectors not allocated at offset 0 bytes (0)\n", which should be the map output for every empty 1 MB image (regardless of format, and so on). Max > + > + # And this is the operation that actually copies data > result = self.vm.qmp('block-stream', device='drive0', base=backing_img) > self.assert_qmp(result, 'return', {})
On Fri 24 Apr 2015 03:19:54 PM CEST, Max Reitz wrote: >> This patch updates test_stream_partial() to test that the >> block-stream operation never copies data from the image specified as >> base. > > I think it would be better to add this as an own test case. But I > won't oppose adding it as a special case of test_stream_partial > either, because strictly speaking, streaming nothing is a special case > of streaming partially. That was the idea, but of course it can be moved to a new test, it's also fine with me. >> + self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img), >> + qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img), >> + 'image file map matches backing file after a no-op') > > Well, you haven't really proven anything by doing this, except that > the mid_img hasn't been streamed completely into test_img... What you > want to prove is that test_img is empty after streaming, right? That's right, but what scenario would that be? A streaming operation that is completed without errors and leaves partial data on the destination image? There's only 512 bytes written in mid_img for this test case. If we suspect that it can go wrong maybe we should start comparing more things than just the qemu_io map output... Berto
On 24.04.2015 15:55, Alberto Garcia wrote: > On Fri 24 Apr 2015 03:19:54 PM CEST, Max Reitz wrote: > >>> This patch updates test_stream_partial() to test that the >>> block-stream operation never copies data from the image specified as >>> base. >> I think it would be better to add this as an own test case. But I >> won't oppose adding it as a special case of test_stream_partial >> either, because strictly speaking, streaming nothing is a special case >> of streaming partially. > That was the idea, but of course it can be moved to a new test, it's > also fine with me. > >>> + self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img), >>> + qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img), >>> + 'image file map matches backing file after a no-op') >> Well, you haven't really proven anything by doing this, except that >> the mid_img hasn't been streamed completely into test_img... What you >> want to prove is that test_img is empty after streaming, right? > That's right, but what scenario would that be? A streaming operation > that is completed without errors and leaves partial data on the > destination image? That would be a bug. Which is what the tests are for. :-) Max > There's only 512 bytes written in mid_img for this test case. If we > suspect that it can go wrong maybe we should start comparing more things > than just the qemu_io map output... > > Berto
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 6e6cb5a..a395a03 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -94,6 +94,19 @@ class TestSingleDrive(iotests.QMPTestCase): def test_stream_partial(self): self.assert_no_active_block_jobs() + # This is a no-op: no data should ever be copied from the base image + result = self.vm.qmp('block-stream', device='drive0', base=mid_img) + self.assert_qmp(result, 'return', {}) + + self.wait_until_completed() + + self.assert_no_active_block_jobs() + + self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', mid_img), + qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img), + 'image file map matches backing file after a no-op') + + # And this is the operation that actually copies data result = self.vm.qmp('block-stream', device='drive0', base=backing_img) self.assert_qmp(result, 'return', {})
This patch updates test_stream_partial() to test that the block-stream operation never copies data from the image specified as base. Signed-off-by: Alberto Garcia <berto@igalia.com> --- tests/qemu-iotests/030 | 13 +++++++++++++ 1 file changed, 13 insertions(+)