[09/11] iotests: test 124 - drive object refactoring

The original test was not particularly good about keeping the
relationships between bitmaps, drives, and images very explicit,
so this patch adds an explicit 'drive' dict that is used to
keep these relationships explicit.

This is necessary in order to test two full backup chains
simultaneously for two drives, which will happen in a forthcoming
test that examines failure scenarios for incremental backups created
for multiple drives in a single transaction.

Highlights:

- Each test case carries around a list of drives
- Each bitmap now acknowledges the full backup belonging to the drive
  as its "last target" if it hasn't made an incremental backup yet.
- Most functions generally try to accept a drive argument instead of
  target or format arguments.
- Filenames are now based on their formatting and id name.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/124 | 212 +++++++++++++++++++++++++++++--------------------
 1 file changed, 126 insertions(+), 86 deletions(-)

On 2015-03-04 at 23:15, John Snow wrote:
> The original test was not particularly good about keeping the
> relationships between bitmaps, drives, and images very explicit,
> so this patch adds an explicit 'drive' dict that is used to
> keep these relationships explicit.
>
> This is necessary in order to test two full backup chains
> simultaneously for two drives, which will happen in a forthcoming
> test that examines failure scenarios for incremental backups created
> for multiple drives in a single transaction.
>
> Highlights:
>
> - Each test case carries around a list of drives
> - Each bitmap now acknowledges the full backup belonging to the drive
>    as its "last target" if it hasn't made an incremental backup yet.
> - Most functions generally try to accept a drive argument instead of
>    target or format arguments.
> - Filenames are now based on their formatting and id name.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/124 | 212 +++++++++++++++++++++++++++++--------------------
>   1 file changed, 126 insertions(+), 86 deletions(-)

How about putting the previous patch in your transactionless series and 
squashing this one into the respective patches there?

Reviewed-by: Max Reitz <mreitz@redhat.com>

Some remarks below, whether you follow them or not is up to you.

> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
> index da0bf6d..2eccc3e 100644
> --- a/tests/qemu-iotests/124
> +++ b/tests/qemu-iotests/124
> @@ -29,14 +29,18 @@ def io_write_patterns(img, patterns):
>           iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
>   
>   class Bitmap:
> -    def __init__(self, name, node):
> +    def __init__(self, name, drive):
>           self.name = name
> -        self.node = node
> +        self.drive = drive
>           self.pattern = os.path.join(iotests.test_dir.replace('%', '%%'),
> -                                    '%s.backup.%%i.img' % name)
> +                                    '%s.%s.backup.%%i.img' % (drive['id'],
> +                                                              name))
>           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
> @@ -46,7 +50,9 @@ class Bitmap:
>           return target
>   
>       def last_target(self):
> -        return self.backups[-1]
> +        if self.backups:
> +            return self.backups[-1]
> +        return self.base_target()
>   
>       def del_target(self):
>           os.remove(self.backups.pop())
> @@ -63,19 +69,35 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>       def setUp(self):
>           self.bitmaps = list()
>           self.files = list()
> +        self.drives = list()
>           self.vm = iotests.VM()
> -        self.test_img = os.path.join(iotests.test_dir, 'base.img')
> -        self.full_bak = os.path.join(iotests.test_dir, 'backup.img')
> -        self.foo_img = os.path.join(iotests.test_dir, 'foo.bar')
> -        self.img_create(self.test_img, iotests.imgfmt)
> -        self.vm.add_drive(self.test_img)
> +        self.err_img = os.path.join(iotests.test_dir, 'err.%s' % iotests.imgfmt)

Nice. *g*

(using the format name as the file extension)

((even though you're not doing it in __init__))

> +
>           # Create a base image with a distinctive patterning
> -        io_write_patterns(self.test_img, (('0x41', 0, 512),
> -                                          ('0xd5', '1M', '32k'),
> -                                          ('0xdc', '32M', '124k')))
> +        drive0 = self.add_node('drive0')
> +        self.img_create(drive0['file'], drive0['fmt'])
> +        self.vm.add_drive(drive0['file'])
> +        io_write_patterns(drive0['file'], (('0x41', 0, 512),
> +                                           ('0xd5', '1M', '32k'),
> +                                           ('0xdc', '32M', '124k')))
>           self.vm.launch()
>   
>   
> +    def add_node(self, node_id, fmt=iotests.imgfmt, path=None, backup=None):
> +        if path is None:
> +            path = os.path.join(iotests.test_dir, '%s.%s' % (node_id, fmt))
> +        if backup is None:
> +            backup = os.path.join(iotests.test_dir,
> +                                  '%s.full.backup.%s' % (node_id, fmt))
> +
> +        self.drives.append({
> +            'id': node_id,
> +            'file': path,
> +            'backup': backup,
> +            'fmt': fmt })
> +        return self.drives[-1]
> +
> +
>       def img_create(self, img, fmt=iotests.imgfmt, size='64M',
>                      parent=None, parentFormat=None):
>           plist = list()
> @@ -89,25 +111,31 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>           self.files.append(img)
>   
>   
> -    def create_full_backup(self, drive='drive0'):
> -        res = self.vm.qmp('drive-backup', device=drive,
> -                          sync='full', format=iotests.imgfmt,
> -                          target=self.full_bak)
> +    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)
> -        self.check_full_backup()
> -        self.files.append(self.full_bak)
> +        self.wait_until_completed(drive['id'])
> +        self.check_full_backup(drive)
> +        self.files.append(drive['backup'])
> +        return drive['backup']
>   
>   
> -    def check_full_backup(self):
> -        self.assertTrue(iotests.compare_images(self.test_img, self.full_bak))
> +    def check_full_backup(self, drive=None):
> +        if drive is None:
> +            drive = self.drives[-1]
> +        self.assertTrue(iotests.compare_images(drive['file'], drive['backup']))
>   
>   
> -    def add_bitmap(self, name, node='drive0'):
> -        bitmap = Bitmap(name, node)
> +    def add_bitmap(self, name, drive):
> +        bitmap = Bitmap(name, drive)
>           self.bitmaps.append(bitmap)
> -        result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node,
> -                             name=bitmap.name)
> +        result = self.vm.qmp('block-dirty-bitmap-add', node=drive['id'],
> +                             name=name)

No real need to replace bitmap.name by name here, but no harm in doing it.

>           self.assert_qmp(result, 'return', {})
>           return bitmap
>   
> @@ -117,37 +145,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>           if bitmap is None:
>               bitmap = self.bitmaps[-1]
>   
> -        # If this is the first incremental backup for a bitmap,
> -        # use the full backup as a backing image. Otherwise, use
> -        # the last incremental backup.
>           if parent is None:
> -            if bitmap.num == 0:
> -                parent = self.full_bak
> -            else:
> -                parent = bitmap.last_target()
> +            parent = bitmap.last_target()
>   
>           target = bitmap.new_target(num)
> -        self.img_create(target, iotests.imgfmt, parent=parent)
> +        self.img_create(target, bitmap.drive['fmt'], parent=parent)
>   
> -        result = self.vm.qmp('drive-backup', device=bitmap.node,
> +        result = self.vm.qmp('drive-backup', device=bitmap.drive['id'],
>                                sync='dirty-bitmap', bitmap=bitmap.name,
> -                             format=iotests.imgfmt, target=target,
> +                             format=bitmap.drive['fmt'], target=target,
>                                mode='existing')
>           self.assert_qmp(result, 'return', {})
>   
> -        event = self.wait_until_completed(bitmap.node, check_offset=validate,
> -                                          allow_failures=(not validate))
> -        if 'error' in event['data']:
> +        return self.wait_incremental(bitmap, validate)
> +
> +
> +    def wait_incremental(self, bitmap=None,
> +                         validate=True, error='Input/output error'):
> +        if bitmap is None:
> +            bitmap = self.bitmaps[-1]
> +
> +        event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED",
> +                                   match={'data': {'device':
> +                                                   bitmap.drive['id']}})
> +        if validate:
> +            self.assert_qmp_absent(event, 'data/error')
> +            return self.check_incremental(bitmap)
> +        else:
> +            self.assert_qmp(event, 'data/error', error)
>               bitmap.del_target()
>               return False
> -        if validate:
> -            return self.check_incremental(target)
>   
>   
> -    def check_incremental(self, target=None):
> -        if target is None:
> -            target = self.bitmaps[-1].last_target()
> -        self.assertTrue(iotests.compare_images(self.test_img, target))
> +    def check_incremental(self, bitmap=None):
> +        if bitmap is None:
> +            bitmap = self.bitmaps[-1]
> +        self.assertTrue(iotests.compare_images(bitmap.drive['file'],
> +                                               bitmap.last_target()))
>           return True
>   
>   
> @@ -166,20 +200,20 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>           i.e.; after IO requests begin modifying the drive.
>           '''
>           self.create_full_backup()
> -        self.add_bitmap('bitmap0', 'drive0')
> +        self.add_bitmap('bitmap0', self.drives[0])
>   
>           # Sanity: Create a "hollow" incremental backup
>           self.create_incremental()
>           # Three writes: One complete overwrite, one new segment,
>           # and one partial overlap.
> -        self.hmp_io_writes('drive0', (('0xab', 0, 512),
> -                                      ('0xfe', '16M', '256k'),
> -                                      ('0x64', '32736k', '64k')))
> +        self.hmp_io_writes(self.drives[0]['id'], (('0xab', 0, 512),
> +                                                  ('0xfe', '16M', '256k'),
> +                                                  ('0x64', '32736k', '64k')))
>           self.create_incremental()
>           # Three more writes, one of each kind, like above
> -        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
> -                                      ('0x55', '8M', '352k'),
> -                                      ('0x78', '15872k', '1M')))
> +        self.hmp_io_writes(self.drives[0]['id'], (('0x9a', 0, 512),
> +                                                  ('0x55', '8M', '352k'),
> +                                                  ('0x78', '15872k', '1M')))
>           self.create_incremental()
>           return True
>   
> @@ -191,41 +225,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>           bitmap AFTER writes have already occurred. Use transactions to create
>           a full backup and synchronize both bitmaps to this backup.
>           Create an incremental backup through both bitmaps and verify that
> -        both backups match the full backup.
> +        both backups match the current drive0 image.
>           '''
> -        bitmap0 = self.add_bitmap('bitmap0', 'drive0')
> -        self.hmp_io_writes('drive0', (('0xab', 0, 512),
> -                                      ('0xfe', '16M', '256k'),
> -                                      ('0x64', '32736k', '64k')))
> -        bitmap1 = self.add_bitmap('bitmap1', 'drive0')
> +
> +        drive0 = self.drives[0]
> +        bitmap0 = self.add_bitmap('bitmap0', self.drives[0])
> +        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
> +                                          ('0xfe', '16M', '256k'),
> +                                          ('0x64', '32736k', '64k')))
> +        bitmap1 = self.add_bitmap('bitmap1', drive0)
>   
>           result = self.vm.qmp('transaction', actions=[
>               {
>                   'type': 'block-dirty-bitmap-clear',
> -                'data': { 'node': 'drive0',
> -                          'name': 'bitmap0' },
> +                'data': { 'node': bitmap0.drive['id'],
> +                          'name': bitmap0.name },
>               },
>               {
>                   'type': 'block-dirty-bitmap-clear',
> -                'data': { 'node': 'drive0',
> -                          'name': 'bitmap1' },
> +                'data': { 'node': bitmap1.drive['id'],
> +                          'name': bitmap1.name },
>               },
>               {
>                   'type': 'drive-backup',
> -                'data': { 'device': 'drive0',
> +                'data': { 'device': drive0['id'],
>                             'sync': 'full',
> -                          'format': iotests.imgfmt,
> -                          'target': self.full_bak },
> +                          'format': drive0['fmt'],
> +                          'target': drive0['backup'] },
>               }
>           ])
>           self.assert_qmp(result, 'return', {})
>           self.wait_until_completed()
> -        self.files.append(self.full_bak)
> +        self.files.append(drive0['backup'])
>           self.check_full_backup()
>   
> -        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
> -                                      ('0x55', '8M', '352k'),
> -                                      ('0x78', '15872k', '1M')))
> +        self.hmp_io_writes(drive0['id'], (('0x9a', 0, 512),
> +                                          ('0x55', '8M', '352k'),
> +                                          ('0x78', '15872k', '1M')))
>           # Both bitmaps should be in sync and create fully valid
>           # incremental backups
>           res1 = self.create_incremental(bitmap0)
> @@ -241,15 +277,19 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>           afterwards and verify that the backup created is correct.
>           '''
>   
> -        # Create a blkdebug interface to this img as 'drive1'
> +        # Create a blkdebug interface to this img as 'drive1',
> +        # but don't actually create a new image.
> +        drive1 = self.add_node('drive1', self.drives[0]['fmt'],
> +                               path=self.drives[0]['file'],
> +                               backup=self.drives[0]['backup'])
>           result = self.vm.qmp('blockdev-add', options={
>               'id': 'drive1',

"'id': drive1['id']" would work, too.

Max

> -            'driver': iotests.imgfmt,
> +            'driver': drive1['fmt'],
>               'file': {
>                   'driver': 'blkdebug',
>                   'image': {
>                       'driver': 'file',
> -                    'filename': self.test_img
> +                    'filename': drive1['file']
>                   },
>                   'set-state': [{
>                       'event': 'flush_to_disk',
> @@ -267,38 +307,38 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>           })
>           self.assert_qmp(result, 'return', {})
>   
> -        self.create_full_backup()
> -        self.add_bitmap('bitmap0', 'drive1')
> +        self.create_full_backup(self.drives[0])
> +        self.add_bitmap('bitmap0', drive1)
>           # Note: at this point, during a normal execution,
>           # Assume that the VM resumes and begins issuing IO requests here.
>   
> -        self.hmp_io_writes('drive1', (('0xab', 0, 512),
> -                                      ('0xfe', '16M', '256k'),
> -                                      ('0x64', '32736k', '64k')))
> +        self.hmp_io_writes(drive1['id'], (('0xab', 0, 512),
> +                                          ('0xfe', '16M', '256k'),
> +                                          ('0x64', '32736k', '64k')))
>   
>           result = self.create_incremental(validate=False)
>           self.assertFalse(result)
> -        self.hmp_io_writes('drive1', (('0x9a', 0, 512),
> -                                      ('0x55', '8M', '352k'),
> -                                      ('0x78', '15872k', '1M')))
> +        self.hmp_io_writes(drive1['id'], (('0x9a', 0, 512),
> +                                          ('0x55', '8M', '352k'),
> +                                          ('0x78', '15872k', '1M')))
>           self.create_incremental()
>   
>   
>       def test_sync_dirty_bitmap_missing(self):
>           self.assert_no_active_block_jobs()
> -        self.files.append(self.foo_img)
> -        result = self.vm.qmp('drive-backup', device='drive0',
> -                             sync='dirty-bitmap', format=iotests.imgfmt,
> -                             target=self.foo_img)
> +        self.files.append(self.err_img)
> +        result = self.vm.qmp('drive-backup', device=self.drives[0]['id'],
> +                             sync='dirty-bitmap', format=self.drives[0]['fmt'],
> +                             target=self.err_img)
>           self.assert_qmp(result, 'error/class', 'GenericError')
>   
>   
>       def test_sync_dirty_bitmap_not_found(self):
>           self.assert_no_active_block_jobs()
> -        self.files.append(self.foo_img)
> -        result = self.vm.qmp('drive-backup', device='drive0',
> +        self.files.append(self.err_img)
> +        result = self.vm.qmp('drive-backup', device=self.drives[0]['id'],
>                                sync='dirty-bitmap', bitmap='unknown',
> -                             format=iotests.imgfmt, target=self.foo_img)
> +                             format=self.drives[0]['fmt'], target=self.err_img)
>           self.assert_qmp(result, 'error/class', 'GenericError')
>   
>

On 03/17/2015 04:44 PM, Max Reitz wrote:
> On 2015-03-04 at 23:15, John Snow wrote:
>> The original test was not particularly good about keeping the
>> relationships between bitmaps, drives, and images very explicit,
>> so this patch adds an explicit 'drive' dict that is used to
>> keep these relationships explicit.
>>
>> This is necessary in order to test two full backup chains
>> simultaneously for two drives, which will happen in a forthcoming
>> test that examines failure scenarios for incremental backups created
>> for multiple drives in a single transaction.
>>
>> Highlights:
>>
>> - Each test case carries around a list of drives
>> - Each bitmap now acknowledges the full backup belonging to the drive
>>    as its "last target" if it hasn't made an incremental backup yet.
>> - Most functions generally try to accept a drive argument instead of
>>    target or format arguments.
>> - Filenames are now based on their formatting and id name.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/124 | 212
>> +++++++++++++++++++++++++++++--------------------
>>   1 file changed, 126 insertions(+), 86 deletions(-)
>
> How about putting the previous patch in your transactionless series and
> squashing this one into the respective patches there?
>

I don't want to startle the cat.


I'd rather get the 'transactionless' checked in, then worry about 
features that are important for transactions in the series that 
introduces those features.

If we keep thinking "Oh, but this function hasn't gone upstream in the 
first place, so let's fix it there" in this series, then the first 
series getting committed is going to wind up dependent on this series 
getting reviewed as we keep changing things.

Since this group of patches is more experimental, I'd really prefer to 
keep it separate that way.

Even if this patch is ugly. Sorry about that.

> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Some remarks below, whether you follow them or not is up to you.
>
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index da0bf6d..2eccc3e 100644
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -29,14 +29,18 @@ def io_write_patterns(img, patterns):
>>           iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
>>   class Bitmap:
>> -    def __init__(self, name, node):
>> +    def __init__(self, name, drive):
>>           self.name = name
>> -        self.node = node
>> +        self.drive = drive
>>           self.pattern = os.path.join(iotests.test_dir.replace('%',
>> '%%'),
>> -                                    '%s.backup.%%i.img' % name)
>> +                                    '%s.%s.backup.%%i.img' %
>> (drive['id'],
>> +                                                              name))
>>           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
>> @@ -46,7 +50,9 @@ class Bitmap:
>>           return target
>>       def last_target(self):
>> -        return self.backups[-1]
>> +        if self.backups:
>> +            return self.backups[-1]
>> +        return self.base_target()
>>       def del_target(self):
>>           os.remove(self.backups.pop())
>> @@ -63,19 +69,35 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>       def setUp(self):
>>           self.bitmaps = list()
>>           self.files = list()
>> +        self.drives = list()
>>           self.vm = iotests.VM()
>> -        self.test_img = os.path.join(iotests.test_dir, 'base.img')
>> -        self.full_bak = os.path.join(iotests.test_dir, 'backup.img')
>> -        self.foo_img = os.path.join(iotests.test_dir, 'foo.bar')
>> -        self.img_create(self.test_img, iotests.imgfmt)
>> -        self.vm.add_drive(self.test_img)
>> +        self.err_img = os.path.join(iotests.test_dir, 'err.%s' %
>> iotests.imgfmt)
>
> Nice. *g*
>
> (using the format name as the file extension)
>
> ((even though you're not doing it in __init__))
>

Can't please everyone. :)

>> +
>>           # Create a base image with a distinctive patterning
>> -        io_write_patterns(self.test_img, (('0x41', 0, 512),
>> -                                          ('0xd5', '1M', '32k'),
>> -                                          ('0xdc', '32M', '124k')))
>> +        drive0 = self.add_node('drive0')
>> +        self.img_create(drive0['file'], drive0['fmt'])
>> +        self.vm.add_drive(drive0['file'])
>> +        io_write_patterns(drive0['file'], (('0x41', 0, 512),
>> +                                           ('0xd5', '1M', '32k'),
>> +                                           ('0xdc', '32M', '124k')))
>>           self.vm.launch()
>> +    def add_node(self, node_id, fmt=iotests.imgfmt, path=None,
>> backup=None):
>> +        if path is None:
>> +            path = os.path.join(iotests.test_dir, '%s.%s' % (node_id,
>> fmt))
>> +        if backup is None:
>> +            backup = os.path.join(iotests.test_dir,
>> +                                  '%s.full.backup.%s' % (node_id, fmt))
>> +
>> +        self.drives.append({
>> +            'id': node_id,
>> +            'file': path,
>> +            'backup': backup,
>> +            'fmt': fmt })
>> +        return self.drives[-1]
>> +
>> +
>>       def img_create(self, img, fmt=iotests.imgfmt, size='64M',
>>                      parent=None, parentFormat=None):
>>           plist = list()
>> @@ -89,25 +111,31 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           self.files.append(img)
>> -    def create_full_backup(self, drive='drive0'):
>> -        res = self.vm.qmp('drive-backup', device=drive,
>> -                          sync='full', format=iotests.imgfmt,
>> -                          target=self.full_bak)
>> +    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)
>> -        self.check_full_backup()
>> -        self.files.append(self.full_bak)
>> +        self.wait_until_completed(drive['id'])
>> +        self.check_full_backup(drive)
>> +        self.files.append(drive['backup'])
>> +        return drive['backup']
>> -    def check_full_backup(self):
>> -        self.assertTrue(iotests.compare_images(self.test_img,
>> self.full_bak))
>> +    def check_full_backup(self, drive=None):
>> +        if drive is None:
>> +            drive = self.drives[-1]
>> +        self.assertTrue(iotests.compare_images(drive['file'],
>> drive['backup']))
>> -    def add_bitmap(self, name, node='drive0'):
>> -        bitmap = Bitmap(name, node)
>> +    def add_bitmap(self, name, drive):
>> +        bitmap = Bitmap(name, drive)
>>           self.bitmaps.append(bitmap)
>> -        result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node,
>> -                             name=bitmap.name)
>> +        result = self.vm.qmp('block-dirty-bitmap-add', node=drive['id'],
>> +                             name=name)
>
> No real need to replace bitmap.name by name here, but no harm in doing it.
>
>>           self.assert_qmp(result, 'return', {})
>>           return bitmap
>> @@ -117,37 +145,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           if bitmap is None:
>>               bitmap = self.bitmaps[-1]
>> -        # If this is the first incremental backup for a bitmap,
>> -        # use the full backup as a backing image. Otherwise, use
>> -        # the last incremental backup.
>>           if parent is None:
>> -            if bitmap.num == 0:
>> -                parent = self.full_bak
>> -            else:
>> -                parent = bitmap.last_target()
>> +            parent = bitmap.last_target()
>>           target = bitmap.new_target(num)
>> -        self.img_create(target, iotests.imgfmt, parent=parent)
>> +        self.img_create(target, bitmap.drive['fmt'], parent=parent)
>> -        result = self.vm.qmp('drive-backup', device=bitmap.node,
>> +        result = self.vm.qmp('drive-backup', device=bitmap.drive['id'],
>>                                sync='dirty-bitmap', bitmap=bitmap.name,
>> -                             format=iotests.imgfmt, target=target,
>> +                             format=bitmap.drive['fmt'], target=target,
>>                                mode='existing')
>>           self.assert_qmp(result, 'return', {})
>> -        event = self.wait_until_completed(bitmap.node,
>> check_offset=validate,
>> -                                          allow_failures=(not validate))
>> -        if 'error' in event['data']:
>> +        return self.wait_incremental(bitmap, validate)
>> +
>> +
>> +    def wait_incremental(self, bitmap=None,
>> +                         validate=True, error='Input/output error'):
>> +        if bitmap is None:
>> +            bitmap = self.bitmaps[-1]
>> +
>> +        event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED",
>> +                                   match={'data': {'device':
>> +                                                   bitmap.drive['id']}})
>> +        if validate:
>> +            self.assert_qmp_absent(event, 'data/error')
>> +            return self.check_incremental(bitmap)
>> +        else:
>> +            self.assert_qmp(event, 'data/error', error)
>>               bitmap.del_target()
>>               return False
>> -        if validate:
>> -            return self.check_incremental(target)
>> -    def check_incremental(self, target=None):
>> -        if target is None:
>> -            target = self.bitmaps[-1].last_target()
>> -        self.assertTrue(iotests.compare_images(self.test_img, target))
>> +    def check_incremental(self, bitmap=None):
>> +        if bitmap is None:
>> +            bitmap = self.bitmaps[-1]
>> +        self.assertTrue(iotests.compare_images(bitmap.drive['file'],
>> +                                               bitmap.last_target()))
>>           return True
>> @@ -166,20 +200,20 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           i.e.; after IO requests begin modifying the drive.
>>           '''
>>           self.create_full_backup()
>> -        self.add_bitmap('bitmap0', 'drive0')
>> +        self.add_bitmap('bitmap0', self.drives[0])
>>           # Sanity: Create a "hollow" incremental backup
>>           self.create_incremental()
>>           # Three writes: One complete overwrite, one new segment,
>>           # and one partial overlap.
>> -        self.hmp_io_writes('drive0', (('0xab', 0, 512),
>> -                                      ('0xfe', '16M', '256k'),
>> -                                      ('0x64', '32736k', '64k')))
>> +        self.hmp_io_writes(self.drives[0]['id'], (('0xab', 0, 512),
>> +                                                  ('0xfe', '16M',
>> '256k'),
>> +                                                  ('0x64', '32736k',
>> '64k')))
>>           self.create_incremental()
>>           # Three more writes, one of each kind, like above
>> -        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
>> -                                      ('0x55', '8M', '352k'),
>> -                                      ('0x78', '15872k', '1M')))
>> +        self.hmp_io_writes(self.drives[0]['id'], (('0x9a', 0, 512),
>> +                                                  ('0x55', '8M',
>> '352k'),
>> +                                                  ('0x78', '15872k',
>> '1M')))
>>           self.create_incremental()
>>           return True
>> @@ -191,41 +225,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           bitmap AFTER writes have already occurred. Use transactions
>> to create
>>           a full backup and synchronize both bitmaps to this backup.
>>           Create an incremental backup through both bitmaps and verify
>> that
>> -        both backups match the full backup.
>> +        both backups match the current drive0 image.
>>           '''
>> -        bitmap0 = self.add_bitmap('bitmap0', 'drive0')
>> -        self.hmp_io_writes('drive0', (('0xab', 0, 512),
>> -                                      ('0xfe', '16M', '256k'),
>> -                                      ('0x64', '32736k', '64k')))
>> -        bitmap1 = self.add_bitmap('bitmap1', 'drive0')
>> +
>> +        drive0 = self.drives[0]
>> +        bitmap0 = self.add_bitmap('bitmap0', self.drives[0])
>> +        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
>> +                                          ('0xfe', '16M', '256k'),
>> +                                          ('0x64', '32736k', '64k')))
>> +        bitmap1 = self.add_bitmap('bitmap1', drive0)
>>           result = self.vm.qmp('transaction', actions=[
>>               {
>>                   'type': 'block-dirty-bitmap-clear',
>> -                'data': { 'node': 'drive0',
>> -                          'name': 'bitmap0' },
>> +                'data': { 'node': bitmap0.drive['id'],
>> +                          'name': bitmap0.name },
>>               },
>>               {
>>                   'type': 'block-dirty-bitmap-clear',
>> -                'data': { 'node': 'drive0',
>> -                          'name': 'bitmap1' },
>> +                'data': { 'node': bitmap1.drive['id'],
>> +                          'name': bitmap1.name },
>>               },
>>               {
>>                   'type': 'drive-backup',
>> -                'data': { 'device': 'drive0',
>> +                'data': { 'device': drive0['id'],
>>                             'sync': 'full',
>> -                          'format': iotests.imgfmt,
>> -                          'target': self.full_bak },
>> +                          'format': drive0['fmt'],
>> +                          'target': drive0['backup'] },
>>               }
>>           ])
>>           self.assert_qmp(result, 'return', {})
>>           self.wait_until_completed()
>> -        self.files.append(self.full_bak)
>> +        self.files.append(drive0['backup'])
>>           self.check_full_backup()
>> -        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
>> -                                      ('0x55', '8M', '352k'),
>> -                                      ('0x78', '15872k', '1M')))
>> +        self.hmp_io_writes(drive0['id'], (('0x9a', 0, 512),
>> +                                          ('0x55', '8M', '352k'),
>> +                                          ('0x78', '15872k', '1M')))
>>           # Both bitmaps should be in sync and create fully valid
>>           # incremental backups
>>           res1 = self.create_incremental(bitmap0)
>> @@ -241,15 +277,19 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           afterwards and verify that the backup created is correct.
>>           '''
>> -        # Create a blkdebug interface to this img as 'drive1'
>> +        # Create a blkdebug interface to this img as 'drive1',
>> +        # but don't actually create a new image.
>> +        drive1 = self.add_node('drive1', self.drives[0]['fmt'],
>> +                               path=self.drives[0]['file'],
>> +                               backup=self.drives[0]['backup'])
>>           result = self.vm.qmp('blockdev-add', options={
>>               'id': 'drive1',
>
> "'id': drive1['id']" would work, too.
>
> Max
>

You're right! I missed one.

>> -            'driver': iotests.imgfmt,
>> +            'driver': drive1['fmt'],
>>               'file': {
>>                   'driver': 'blkdebug',
>>                   'image': {
>>                       'driver': 'file',
>> -                    'filename': self.test_img
>> +                    'filename': drive1['file']
>>                   },
>>                   'set-state': [{
>>                       'event': 'flush_to_disk',
>> @@ -267,38 +307,38 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           })
>>           self.assert_qmp(result, 'return', {})
>> -        self.create_full_backup()
>> -        self.add_bitmap('bitmap0', 'drive1')
>> +        self.create_full_backup(self.drives[0])
>> +        self.add_bitmap('bitmap0', drive1)
>>           # Note: at this point, during a normal execution,
>>           # Assume that the VM resumes and begins issuing IO requests
>> here.
>> -        self.hmp_io_writes('drive1', (('0xab', 0, 512),
>> -                                      ('0xfe', '16M', '256k'),
>> -                                      ('0x64', '32736k', '64k')))
>> +        self.hmp_io_writes(drive1['id'], (('0xab', 0, 512),
>> +                                          ('0xfe', '16M', '256k'),
>> +                                          ('0x64', '32736k', '64k')))
>>           result = self.create_incremental(validate=False)
>>           self.assertFalse(result)
>> -        self.hmp_io_writes('drive1', (('0x9a', 0, 512),
>> -                                      ('0x55', '8M', '352k'),
>> -                                      ('0x78', '15872k', '1M')))
>> +        self.hmp_io_writes(drive1['id'], (('0x9a', 0, 512),
>> +                                          ('0x55', '8M', '352k'),
>> +                                          ('0x78', '15872k', '1M')))
>>           self.create_incremental()
>>       def test_sync_dirty_bitmap_missing(self):
>>           self.assert_no_active_block_jobs()
>> -        self.files.append(self.foo_img)
>> -        result = self.vm.qmp('drive-backup', device='drive0',
>> -                             sync='dirty-bitmap', format=iotests.imgfmt,
>> -                             target=self.foo_img)
>> +        self.files.append(self.err_img)
>> +        result = self.vm.qmp('drive-backup',
>> device=self.drives[0]['id'],
>> +                             sync='dirty-bitmap',
>> format=self.drives[0]['fmt'],
>> +                             target=self.err_img)
>>           self.assert_qmp(result, 'error/class', 'GenericError')
>>       def test_sync_dirty_bitmap_not_found(self):
>>           self.assert_no_active_block_jobs()
>> -        self.files.append(self.foo_img)
>> -        result = self.vm.qmp('drive-backup', device='drive0',
>> +        self.files.append(self.err_img)
>> +        result = self.vm.qmp('drive-backup',
>> device=self.drives[0]['id'],
>>                                sync='dirty-bitmap', bitmap='unknown',
>> -                             format=iotests.imgfmt, target=self.foo_img)
>> +                             format=self.drives[0]['fmt'],
>> target=self.err_img)
>>           self.assert_qmp(result, 'error/class', 'GenericError')
>
>

On 2015-03-17 at 19:40, John Snow wrote:
>
>
> On 03/17/2015 04:44 PM, Max Reitz wrote:
>> On 2015-03-04 at 23:15, John Snow wrote:
>>> The original test was not particularly good about keeping the
>>> relationships between bitmaps, drives, and images very explicit,
>>> so this patch adds an explicit 'drive' dict that is used to
>>> keep these relationships explicit.
>>>
>>> This is necessary in order to test two full backup chains
>>> simultaneously for two drives, which will happen in a forthcoming
>>> test that examines failure scenarios for incremental backups created
>>> for multiple drives in a single transaction.
>>>
>>> Highlights:
>>>
>>> - Each test case carries around a list of drives
>>> - Each bitmap now acknowledges the full backup belonging to the drive
>>>    as its "last target" if it hasn't made an incremental backup yet.
>>> - Most functions generally try to accept a drive argument instead of
>>>    target or format arguments.
>>> - Filenames are now based on their formatting and id name.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   tests/qemu-iotests/124 | 212
>>> +++++++++++++++++++++++++++++--------------------
>>>   1 file changed, 126 insertions(+), 86 deletions(-)
>>
>> How about putting the previous patch in your transactionless series and
>> squashing this one into the respective patches there?
>>
>
> I don't want to startle the cat.
>
>
> I'd rather get the 'transactionless' checked in, then worry about 
> features that are important for transactions in the series that 
> introduces those features.
>
> If we keep thinking "Oh, but this function hasn't gone upstream in the 
> first place, so let's fix it there" in this series, then the first 
> series getting committed is going to wind up dependent on this series 
> getting reviewed as we keep changing things.
>
> Since this group of patches is more experimental, I'd really prefer to 
> keep it separate that way.
>
> Even if this patch is ugly. Sorry about that.

Well, you're targetting 2.4 with your other series anyway. I understand 
that it's been a couple of revisions already and you really want to get 
it merged by now, but this is a test, so if you just squash it in there, 
I don't think it will hold up that series much.

(A good rule for tests is: As long as they're passing and catch all 
desirable cases, nobody will really complain)

Up to you, but I guess you can see how this patch just looks like "I 
should have done it differently in the first place", and because that 
"first place" is still in flight, I'm wondering why you're not just 
fixing it there.

Max

>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> Some remarks below, whether you follow them or not is up to you.
>>
>>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>>> index da0bf6d..2eccc3e 100644
>>> --- a/tests/qemu-iotests/124
>>> +++ b/tests/qemu-iotests/124
>>> @@ -29,14 +29,18 @@ def io_write_patterns(img, patterns):
>>>           iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
>>>   class Bitmap:
>>> -    def __init__(self, name, node):
>>> +    def __init__(self, name, drive):
>>>           self.name = name
>>> -        self.node = node
>>> +        self.drive = drive
>>>           self.pattern = os.path.join(iotests.test_dir.replace('%',
>>> '%%'),
>>> -                                    '%s.backup.%%i.img' % name)
>>> +                                    '%s.%s.backup.%%i.img' %
>>> (drive['id'],
>>> + name))
>>>           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
>>> @@ -46,7 +50,9 @@ class Bitmap:
>>>           return target
>>>       def last_target(self):
>>> -        return self.backups[-1]
>>> +        if self.backups:
>>> +            return self.backups[-1]
>>> +        return self.base_target()
>>>       def del_target(self):
>>>           os.remove(self.backups.pop())
>>> @@ -63,19 +69,35 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>       def setUp(self):
>>>           self.bitmaps = list()
>>>           self.files = list()
>>> +        self.drives = list()
>>>           self.vm = iotests.VM()
>>> -        self.test_img = os.path.join(iotests.test_dir, 'base.img')
>>> -        self.full_bak = os.path.join(iotests.test_dir, 'backup.img')
>>> -        self.foo_img = os.path.join(iotests.test_dir, 'foo.bar')
>>> -        self.img_create(self.test_img, iotests.imgfmt)
>>> -        self.vm.add_drive(self.test_img)
>>> +        self.err_img = os.path.join(iotests.test_dir, 'err.%s' %
>>> iotests.imgfmt)
>>
>> Nice. *g*
>>
>> (using the format name as the file extension)
>>
>> ((even though you're not doing it in __init__))
>>
>
> Can't please everyone. :)
>
>>> +
>>>           # Create a base image with a distinctive patterning
>>> -        io_write_patterns(self.test_img, (('0x41', 0, 512),
>>> -                                          ('0xd5', '1M', '32k'),
>>> -                                          ('0xdc', '32M', '124k')))
>>> +        drive0 = self.add_node('drive0')
>>> +        self.img_create(drive0['file'], drive0['fmt'])
>>> +        self.vm.add_drive(drive0['file'])
>>> +        io_write_patterns(drive0['file'], (('0x41', 0, 512),
>>> +                                           ('0xd5', '1M', '32k'),
>>> +                                           ('0xdc', '32M', '124k')))
>>>           self.vm.launch()
>>> +    def add_node(self, node_id, fmt=iotests.imgfmt, path=None,
>>> backup=None):
>>> +        if path is None:
>>> +            path = os.path.join(iotests.test_dir, '%s.%s' % (node_id,
>>> fmt))
>>> +        if backup is None:
>>> +            backup = os.path.join(iotests.test_dir,
>>> +                                  '%s.full.backup.%s' % (node_id, 
>>> fmt))
>>> +
>>> +        self.drives.append({
>>> +            'id': node_id,
>>> +            'file': path,
>>> +            'backup': backup,
>>> +            'fmt': fmt })
>>> +        return self.drives[-1]
>>> +
>>> +
>>>       def img_create(self, img, fmt=iotests.imgfmt, size='64M',
>>>                      parent=None, parentFormat=None):
>>>           plist = list()
>>> @@ -89,25 +111,31 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>           self.files.append(img)
>>> -    def create_full_backup(self, drive='drive0'):
>>> -        res = self.vm.qmp('drive-backup', device=drive,
>>> -                          sync='full', format=iotests.imgfmt,
>>> -                          target=self.full_bak)
>>> +    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)
>>> -        self.check_full_backup()
>>> -        self.files.append(self.full_bak)
>>> +        self.wait_until_completed(drive['id'])
>>> +        self.check_full_backup(drive)
>>> +        self.files.append(drive['backup'])
>>> +        return drive['backup']
>>> -    def check_full_backup(self):
>>> -        self.assertTrue(iotests.compare_images(self.test_img,
>>> self.full_bak))
>>> +    def check_full_backup(self, drive=None):
>>> +        if drive is None:
>>> +            drive = self.drives[-1]
>>> +        self.assertTrue(iotests.compare_images(drive['file'],
>>> drive['backup']))
>>> -    def add_bitmap(self, name, node='drive0'):
>>> -        bitmap = Bitmap(name, node)
>>> +    def add_bitmap(self, name, drive):
>>> +        bitmap = Bitmap(name, drive)
>>>           self.bitmaps.append(bitmap)
>>> -        result = self.vm.qmp('block-dirty-bitmap-add', 
>>> node=bitmap.node,
>>> -                             name=bitmap.name)
>>> +        result = self.vm.qmp('block-dirty-bitmap-add', 
>>> node=drive['id'],
>>> +                             name=name)
>>
>> No real need to replace bitmap.name by name here, but no harm in 
>> doing it.
>>
>>>           self.assert_qmp(result, 'return', {})
>>>           return bitmap
>>> @@ -117,37 +145,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>           if bitmap is None:
>>>               bitmap = self.bitmaps[-1]
>>> -        # If this is the first incremental backup for a bitmap,
>>> -        # use the full backup as a backing image. Otherwise, use
>>> -        # the last incremental backup.
>>>           if parent is None:
>>> -            if bitmap.num == 0:
>>> -                parent = self.full_bak
>>> -            else:
>>> -                parent = bitmap.last_target()
>>> +            parent = bitmap.last_target()
>>>           target = bitmap.new_target(num)
>>> -        self.img_create(target, iotests.imgfmt, parent=parent)
>>> +        self.img_create(target, bitmap.drive['fmt'], parent=parent)
>>> -        result = self.vm.qmp('drive-backup', device=bitmap.node,
>>> +        result = self.vm.qmp('drive-backup', 
>>> device=bitmap.drive['id'],
>>>                                sync='dirty-bitmap', bitmap=bitmap.name,
>>> -                             format=iotests.imgfmt, target=target,
>>> +                             format=bitmap.drive['fmt'], 
>>> target=target,
>>>                                mode='existing')
>>>           self.assert_qmp(result, 'return', {})
>>> -        event = self.wait_until_completed(bitmap.node,
>>> check_offset=validate,
>>> -                                          allow_failures=(not 
>>> validate))
>>> -        if 'error' in event['data']:
>>> +        return self.wait_incremental(bitmap, validate)
>>> +
>>> +
>>> +    def wait_incremental(self, bitmap=None,
>>> +                         validate=True, error='Input/output error'):
>>> +        if bitmap is None:
>>> +            bitmap = self.bitmaps[-1]
>>> +
>>> +        event = self.vm.event_wait(name="BLOCK_JOB_COMPLETED",
>>> +                                   match={'data': {'device':
>>> + bitmap.drive['id']}})
>>> +        if validate:
>>> +            self.assert_qmp_absent(event, 'data/error')
>>> +            return self.check_incremental(bitmap)
>>> +        else:
>>> +            self.assert_qmp(event, 'data/error', error)
>>>               bitmap.del_target()
>>>               return False
>>> -        if validate:
>>> -            return self.check_incremental(target)
>>> -    def check_incremental(self, target=None):
>>> -        if target is None:
>>> -            target = self.bitmaps[-1].last_target()
>>> -        self.assertTrue(iotests.compare_images(self.test_img, target))
>>> +    def check_incremental(self, bitmap=None):
>>> +        if bitmap is None:
>>> +            bitmap = self.bitmaps[-1]
>>> + self.assertTrue(iotests.compare_images(bitmap.drive['file'],
>>> + bitmap.last_target()))
>>>           return True
>>> @@ -166,20 +200,20 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>           i.e.; after IO requests begin modifying the drive.
>>>           '''
>>>           self.create_full_backup()
>>> -        self.add_bitmap('bitmap0', 'drive0')
>>> +        self.add_bitmap('bitmap0', self.drives[0])
>>>           # Sanity: Create a "hollow" incremental backup
>>>           self.create_incremental()
>>>           # Three writes: One complete overwrite, one new segment,
>>>           # and one partial overlap.
>>> -        self.hmp_io_writes('drive0', (('0xab', 0, 512),
>>> -                                      ('0xfe', '16M', '256k'),
>>> -                                      ('0x64', '32736k', '64k')))
>>> +        self.hmp_io_writes(self.drives[0]['id'], (('0xab', 0, 512),
>>> +                                                  ('0xfe', '16M',
>>> '256k'),
>>> +                                                  ('0x64', '32736k',
>>> '64k')))
>>>           self.create_incremental()
>>>           # Three more writes, one of each kind, like above
>>> -        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
>>> -                                      ('0x55', '8M', '352k'),
>>> -                                      ('0x78', '15872k', '1M')))
>>> +        self.hmp_io_writes(self.drives[0]['id'], (('0x9a', 0, 512),
>>> +                                                  ('0x55', '8M',
>>> '352k'),
>>> +                                                  ('0x78', '15872k',
>>> '1M')))
>>>           self.create_incremental()
>>>           return True
>>> @@ -191,41 +225,43 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>           bitmap AFTER writes have already occurred. Use transactions
>>> to create
>>>           a full backup and synchronize both bitmaps to this backup.
>>>           Create an incremental backup through both bitmaps and verify
>>> that
>>> -        both backups match the full backup.
>>> +        both backups match the current drive0 image.
>>>           '''
>>> -        bitmap0 = self.add_bitmap('bitmap0', 'drive0')
>>> -        self.hmp_io_writes('drive0', (('0xab', 0, 512),
>>> -                                      ('0xfe', '16M', '256k'),
>>> -                                      ('0x64', '32736k', '64k')))
>>> -        bitmap1 = self.add_bitmap('bitmap1', 'drive0')
>>> +
>>> +        drive0 = self.drives[0]
>>> +        bitmap0 = self.add_bitmap('bitmap0', self.drives[0])
>>> +        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
>>> +                                          ('0xfe', '16M', '256k'),
>>> +                                          ('0x64', '32736k', '64k')))
>>> +        bitmap1 = self.add_bitmap('bitmap1', drive0)
>>>           result = self.vm.qmp('transaction', actions=[
>>>               {
>>>                   'type': 'block-dirty-bitmap-clear',
>>> -                'data': { 'node': 'drive0',
>>> -                          'name': 'bitmap0' },
>>> +                'data': { 'node': bitmap0.drive['id'],
>>> +                          'name': bitmap0.name },
>>>               },
>>>               {
>>>                   'type': 'block-dirty-bitmap-clear',
>>> -                'data': { 'node': 'drive0',
>>> -                          'name': 'bitmap1' },
>>> +                'data': { 'node': bitmap1.drive['id'],
>>> +                          'name': bitmap1.name },
>>>               },
>>>               {
>>>                   'type': 'drive-backup',
>>> -                'data': { 'device': 'drive0',
>>> +                'data': { 'device': drive0['id'],
>>>                             'sync': 'full',
>>> -                          'format': iotests.imgfmt,
>>> -                          'target': self.full_bak },
>>> +                          'format': drive0['fmt'],
>>> +                          'target': drive0['backup'] },
>>>               }
>>>           ])
>>>           self.assert_qmp(result, 'return', {})
>>>           self.wait_until_completed()
>>> -        self.files.append(self.full_bak)
>>> +        self.files.append(drive0['backup'])
>>>           self.check_full_backup()
>>> -        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
>>> -                                      ('0x55', '8M', '352k'),
>>> -                                      ('0x78', '15872k', '1M')))
>>> +        self.hmp_io_writes(drive0['id'], (('0x9a', 0, 512),
>>> +                                          ('0x55', '8M', '352k'),
>>> +                                          ('0x78', '15872k', '1M')))
>>>           # Both bitmaps should be in sync and create fully valid
>>>           # incremental backups
>>>           res1 = self.create_incremental(bitmap0)
>>> @@ -241,15 +277,19 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>           afterwards and verify that the backup created is correct.
>>>           '''
>>> -        # Create a blkdebug interface to this img as 'drive1'
>>> +        # Create a blkdebug interface to this img as 'drive1',
>>> +        # but don't actually create a new image.
>>> +        drive1 = self.add_node('drive1', self.drives[0]['fmt'],
>>> +                               path=self.drives[0]['file'],
>>> + backup=self.drives[0]['backup'])
>>>           result = self.vm.qmp('blockdev-add', options={
>>>               'id': 'drive1',
>>
>> "'id': drive1['id']" would work, too.
>>
>> Max
>>
>
> You're right! I missed one.
>
>>> -            'driver': iotests.imgfmt,
>>> +            'driver': drive1['fmt'],
>>>               'file': {
>>>                   'driver': 'blkdebug',
>>>                   'image': {
>>>                       'driver': 'file',
>>> -                    'filename': self.test_img
>>> +                    'filename': drive1['file']
>>>                   },
>>>                   'set-state': [{
>>>                       'event': 'flush_to_disk',
>>> @@ -267,38 +307,38 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>>           })
>>>           self.assert_qmp(result, 'return', {})
>>> -        self.create_full_backup()
>>> -        self.add_bitmap('bitmap0', 'drive1')
>>> +        self.create_full_backup(self.drives[0])
>>> +        self.add_bitmap('bitmap0', drive1)
>>>           # Note: at this point, during a normal execution,
>>>           # Assume that the VM resumes and begins issuing IO requests
>>> here.
>>> -        self.hmp_io_writes('drive1', (('0xab', 0, 512),
>>> -                                      ('0xfe', '16M', '256k'),
>>> -                                      ('0x64', '32736k', '64k')))
>>> +        self.hmp_io_writes(drive1['id'], (('0xab', 0, 512),
>>> +                                          ('0xfe', '16M', '256k'),
>>> +                                          ('0x64', '32736k', '64k')))
>>>           result = self.create_incremental(validate=False)
>>>           self.assertFalse(result)
>>> -        self.hmp_io_writes('drive1', (('0x9a', 0, 512),
>>> -                                      ('0x55', '8M', '352k'),
>>> -                                      ('0x78', '15872k', '1M')))
>>> +        self.hmp_io_writes(drive1['id'], (('0x9a', 0, 512),
>>> +                                          ('0x55', '8M', '352k'),
>>> +                                          ('0x78', '15872k', '1M')))
>>>           self.create_incremental()
>>>       def test_sync_dirty_bitmap_missing(self):
>>>           self.assert_no_active_block_jobs()
>>> -        self.files.append(self.foo_img)
>>> -        result = self.vm.qmp('drive-backup', device='drive0',
>>> -                             sync='dirty-bitmap', 
>>> format=iotests.imgfmt,
>>> -                             target=self.foo_img)
>>> +        self.files.append(self.err_img)
>>> +        result = self.vm.qmp('drive-backup',
>>> device=self.drives[0]['id'],
>>> +                             sync='dirty-bitmap',
>>> format=self.drives[0]['fmt'],
>>> +                             target=self.err_img)
>>>           self.assert_qmp(result, 'error/class', 'GenericError')
>>>       def test_sync_dirty_bitmap_not_found(self):
>>>           self.assert_no_active_block_jobs()
>>> -        self.files.append(self.foo_img)
>>> -        result = self.vm.qmp('drive-backup', device='drive0',
>>> +        self.files.append(self.err_img)
>>> +        result = self.vm.qmp('drive-backup',
>>> device=self.drives[0]['id'],
>>>                                sync='dirty-bitmap', bitmap='unknown',
>>> -                             format=iotests.imgfmt, 
>>> target=self.foo_img)
>>> +                             format=self.drives[0]['fmt'],
>>> target=self.err_img)
>>>           self.assert_qmp(result, 'error/class', 'GenericError')
>>
>>