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

[John Snow <jsnow@redhat.com>]

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.

--js