From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52372) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXyRs-0007WQ-Km for qemu-devel@nongnu.org; Tue, 17 Mar 2015 16:50:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXyRo-0001zV-Kc for qemu-devel@nongnu.org; Tue, 17 Mar 2015 16:50:48 -0400 Message-ID: <550893A1.2070007@redhat.com> Date: Tue, 17 Mar 2015 16:50:41 -0400 From: Max Reitz MIME-Version: 1.0 References: <1425528911-10300-1-git-send-email-jsnow@redhat.com> <1425528911-10300-11-git-send-email-jsnow@redhat.com> In-Reply-To: <1425528911-10300-11-git-send-email-jsnow@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: John Snow , 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 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? > 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