[10/11] iotests: 124 - backup_prepare refactoring

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

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?

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

>           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>

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!