Re: [Qemu-devel] [PATCH 10/11] iotests: 124 - backup_prepare refactoring

[John Snow <jsnow@redhat.com>]

On 03/17/2015 04:50 PM, Max Reitz wrote:
> On 2015-03-04 at 23:15, John Snow wrote:
>> Allow tests to call just the backup preparation routine
>> without invoking a backup. Useful for transactions where
>> we want to prepare, but don't wish to issue the QMP command.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/124 | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 2eccc3e..4afdca1 100644
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -100,7 +100,6 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>       def img_create(self, img, fmt=iotests.imgfmt, size='64M',
>>                      parent=None, parentFormat=None):
>> -        plist = list()
>
> Hm. This appears to never have been used. Would it make sense to drop it
> from "iotests: add invalid input incremental backup tests" in your
> transactionless series?
>

Yes. (Poor kitty.)

>>           if parent:
>>               if parentFormat is None:
>>                   parentFormat = fmt
>> @@ -140,17 +139,25 @@ class TestIncrementalBackup(iotests.QMPTestCase):
>>           return bitmap
>> -    def create_incremental(self, bitmap=None, num=None,
>> -                           parent=None, parentFormat=None,
>> validate=True):
>> +    def prepare_backup(self, bitmap=None, parent=None):
>>           if bitmap is None:
>>               bitmap = self.bitmaps[-1]
>> -
>
> Removing this empty line looks like it should never have been added, too.
>

Also yes.

>>           if parent is None:
>>               parent = bitmap.last_target()
>> -        target = bitmap.new_target(num)
>> +        target = bitmap.new_target()
>>           self.img_create(target, bitmap.drive['fmt'], parent=parent)
>> +        return target
>> +
>> +    def create_incremental(self, bitmap=None, parent=None,
>> +                           parentFormat=None, validate=True):
>> +        if bitmap is None:
>> +            bitmap = self.bitmaps[-1]
>> +        if parent is None:
>> +            parent = bitmap.last_target()
>> +
>> +        target = self.prepare_backup(bitmap, parent)
>>           result = self.vm.qmp('drive-backup', device=bitmap.drive['id'],
>>                                sync='dirty-bitmap', bitmap=bitmap.name,
>>                                format=bitmap.drive['fmt'], target=target,
>
> Both "issues" are not caused by this patch, however, so whether you
> squash the hunks somewhere else or not:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>

Thanks!