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

[John Snow]

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 <address@hidden>
> > ---
> >   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