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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3913 bytes --]

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
> @@ -28,6 +28,42 @@ def io_write_patterns(img, patterns):
>      for pattern in patterns:
>          iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
>  
> +class Bitmap:
> +    def __init__(self, name, drive):
> +        self.name = name
> +        self.drive = drive
> +        self.pattern = os.path.join(iotests.test_dir.replace('%', '%%'),
> +                                    '%s.%s.backup.%%i.img' % (drive['id'],
> +                                                              name))

drive['id'] and name aren't escaped.

It is simpler and safer to format the string from scratch in
new_target() and drop the Bitmap.pattern field:

  def new_target(self, num=None):
      ...
      filename = '%s.%s.backup.%i.img' % (self.drive['id'],
                                          self.name,
                                          num)
      target = os.path.join(iotests.test_dir, filename)

> +        self.num = 0
> +        self.backups = list()
> +
> +    def base_target(self):
> +        return self.drive['backup']
> +
> +    def new_target(self, num=None):
> +        if num is None:
> +            num = self.num
> +        self.num = num + 1
> +        target = self.pattern % num
> +        self.backups.append(target)
> +        return target
> +
> +    def last_target(self):
> +        if self.backups:
> +            return self.backups[-1]
> +        return self.base_target()
> +
> +    def del_target(self):
> +        os.remove(self.backups.pop())
> +        self.num -= 1
> +
> +    def __del__(self):
> +        for backup in self.backups:
> +            try:
> +                os.remove(backup)
> +            except OSError:
> +                pass

From the language reference:

  "It is not guaranteed that __del__() methods are called for objects
  that still exist when the interpreter exits."

https://docs.python.org/2.7/reference/datamodel.html#object.__del__

Relying on reference counts is risky.

The TestCase.tearDown() method is the place to clean up:
https://docs.python.org/2.7/library/unittest.html#unittest.TestCase.tearDown

It could iterate over self.bitmaps[] and call a cleanup() function.

>  
>  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().

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]