From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36070) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YY19g-00063o-T7 for qemu-devel@nongnu.org; Tue, 17 Mar 2015 19:44:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YY19g-00010f-3U for qemu-devel@nongnu.org; Tue, 17 Mar 2015 19:44:12 -0400 Message-ID: <5508BC44.2020505@redhat.com> Date: Tue, 17 Mar 2015 19:44:04 -0400 From: John Snow MIME-Version: 1.0 References: <1425528911-10300-1-git-send-email-jsnow@redhat.com> <1425528911-10300-11-git-send-email-jsnow@redhat.com> <550893A1.2070007@redhat.com> In-Reply-To: <550893A1.2070007@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/11] iotests: 124 - backup_prepare refactoring List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@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 >> --- >> 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 > Thanks!