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

[Max Reitz <mreitz@redhat.com>]

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>