Re: [Qemu-devel] [PATCH v4 19/20] iotests: add simple incremental backup case

[Max Reitz <mreitz@redhat.com>]

On 06.04.2015 23:49, John Snow wrote:
>
>
> On 04/02/2015 10:27 AM, Stefan Hajnoczi wrote:
>> On Fri, Mar 20, 2015 at 03:17:02PM -0400, John Snow wrote:
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   tests/qemu-iotests/124     | 153 
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/124.out |   4 +-
>>>   2 files changed, 155 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>>> index 85675ec..ce2cda7 100644
>>> --- a/tests/qemu-iotests/124
>>> +++ b/tests/qemu-iotests/124
>
> [snip, snip, snip]
>
>>>
>>>   class TestIncrementalBackup(iotests.QMPTestCase):
>>>       def setUp(self):
>>> @@ -73,6 +109,123 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>               iotests.qemu_img('create', '-f', fmt, img, size)
>>>           self.files.append(img)
>>>
>>> +
>>> +    def create_full_backup(self, drive=None):
>>> +        if drive is None:
>>> +            drive = self.drives[-1]
>>> +
>>> +        res = self.vm.qmp('drive-backup', device=drive['id'],
>>> +                          sync='full', format=drive['fmt'],
>>> +                          target=drive['backup'])
>>> +        self.assert_qmp(res, 'return', {})
>>> +        self.wait_until_completed(drive['id'])
>>> +        self.check_full_backup(drive)
>>> +        self.files.append(drive['backup'])
>>> +        return drive['backup']
>>> +
>>> +
>>> +    def check_full_backup(self, drive=None):
>>> +        if drive is None:
>>> +            drive = self.drives[-1]
>>> +        self.assertTrue(iotests.compare_images(drive['file'], 
>>> drive['backup']))
>>
>> I think QEMU still has at least drive['file'] open?  It's not safe to
>> access the file from another program while it is open.
>>
>> The simplest solution is to terminate the VM before calling
>> iotests.compare_images().
>>
>
> Oh, that's unfortunate. I have been checking images after every 
> creation as a nice incremental sanity check. That will be hard to do 
> if I have to actually close QEMU.
>
> My reasoning was:
>
> (1) We're explicitly flushing after every write,
> (2) We're in a qtest mode so there is no guest activity,
> (3) Nobody is writing to this image by the time we call compare_images().
>
> So it should be safe to /read/ the files while QEMU is occupied doing 
> nothing.
>
> I could delay all tests until completion, but I'd lose out on the 
> ability to test the equivalence of any incrementals that are not their 
> most recent versions, unless I also start creating a lot of full 
> backups, but I think this starts to get messy and makes the tests hard 
> to follow.
>
> Is it really that unsafe? I could add in an explicit pause/resume 
> barrier around the check if that would help inspire some confidence in 
> the test.

I'm fine with it (and consciously accepted it while reviewing), because 
the worst I can see is the test breaking. You're modifying the image and 
you're checking whether the modifications are appearing in the image. I 
cannot really see how qemu could not have made the modifications or made 
them differently than expected, and they still appear in the image file, 
just as expected.

Second, we are already relying on internal things pretty much in the 
iotests (just take qemu-io, it's nothing users should ever use). So 
because relying on things users shouldn't do is already a part of the 
iotests and because if it doesn't work, the test will most likely just 
break, I'm fine with it.


In anticipation of an "Yes, *you* can't see how the behavior can break, 
making the test pass when it's actually failed, but it's not impossible, 
you're relying on undefined behavior after all! Doing this even in just 
a test might consume your HDD in a dragon fire!",

Max