From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLf1e-0001fe-4I for qemu-devel@nongnu.org; Wed, 11 Feb 2015 16:40:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLf1Z-0005c9-3n for qemu-devel@nongnu.org; Wed, 11 Feb 2015 16:40:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37317) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLf1Y-0005ba-Sj for qemu-devel@nongnu.org; Wed, 11 Feb 2015 16:40:45 -0500 Message-ID: <54DBCC59.7060303@redhat.com> Date: Wed, 11 Feb 2015 16:40:41 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423532117-14490-1-git-send-email-jsnow@redhat.com> <1423532117-14490-15-git-send-email-jsnow@redhat.com> In-Reply-To: <1423532117-14490-15-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 v12 14/17] iotests: add simple incremental backup case List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 2015-02-09 at 20:35, John Snow wrote: > Signed-off-by: John Snow > --- > tests/qemu-iotests/112 | 120 +++++++++++++++++++++++++++++++++++++++++- > tests/qemu-iotests/112.out | 4 +- > tests/qemu-iotests/iotests.py | 18 ++++--- > 3 files changed, 133 insertions(+), 9 deletions(-) > > diff --git a/tests/qemu-iotests/112 b/tests/qemu-iotests/112 > index 7985cd1..31431ad 100644 > --- a/tests/qemu-iotests/112 > +++ b/tests/qemu-iotests/112 > @@ -22,18 +22,49 @@ > > import os > import iotests > +from iotests import qemu_img, qemu_io, create_image Is this really necessary? For me, it works without, too; and if it is necessary, shouldn't it be part of patch 13? > def io_write_patterns(img, patterns): > for pattern in patterns: > iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img) > > +class Bitmap: > + def __init__(self, name, node): > + self.name = name > + self.node = node > + self.pattern = os.path.join(iotests.test_dir, > + '%s.backup.%%s.img' % name) Shouldn't this be %%i? %%s works for me, but I think %%i might look better. > + self.num = 0 > + self.backups = list() > + > + def new_target(self, num=None): > + if num is None: > + num = self.num > + self.num = num + 1 > + target = self.pattern % num I hope nobody has a % in the test path... Although it does look nice, maybe just self.base = os.path.join(iotests.test_dir, '%s.backup.img.' % name) and target = self.base + str(num) is more robust (putting the '.img' at the end is a bonus). > + self.backups.append(target) > + return target > + > + def last_target(self): > + return self.backups[-1] > + > + def del_target(self): > + os.remove(self.backups.pop()) > + self.num -= 1 > + > + def __del__(self): > + for backup in self.backups: > + try: > + os.remove(backup) > + except OSError: > + pass > > class TestIncrementalBackup(iotests.QMPTestCase): > def setUp(self): > self.bitmaps = list() > self.files = list() > - self.vm = iotests.VM() > + self.vm = iotests.VM().add_args(['-S']) Isn't -machine accel=qtest (which is specified by default) enough? > self.test_img = os.path.join(iotests.test_dir, 'base.img') > self.full_bak = os.path.join(iotests.test_dir, 'backup.img') > self.foo_img = os.path.join(iotests.test_dir, 'foo.bar') > @@ -58,6 +89,93 @@ class TestIncrementalBackup(iotests.QMPTestCase): > iotests.qemu_img('create', '-f', fmt, img, size) > self.files.append(img) > > + > + def create_full_backup(self, drive='drive0'): > + res = self.vm.qmp('drive-backup', device=drive, > + sync='full', format=iotests.imgfmt, > + target=self.full_bak) > + self.assert_qmp(res, 'return', {}) > + self.wait_until_completed(drive) > + self.check_full_backup() > + self.files.append(self.full_bak) > + > + > + def check_full_backup(self): > + self.assertTrue(iotests.compare_images(self.test_img, self.full_bak)) > + > + > + def add_bitmap(self, name, node='drive0'): > + bitmap = Bitmap(name, node) > + self.bitmaps.append(bitmap) > + result = self.vm.qmp('block-dirty-bitmap-add', node=bitmap.node, > + name=bitmap.name) > + self.assert_qmp(result, 'return', {}) > + return bitmap > + > + > + def create_incremental(self, bitmap=None, num=None, > + parent=None, parentFormat=None, validate=True): > + if bitmap is None: > + bitmap = self.bitmaps[-1] > + > + # If this is the first incremental backup for a bitmap, > + # use the full backup as a backing image. Otherwise, use > + # the last incremental backup. > + if parent is None: > + if bitmap.num is 0: Why not == 0? > + parent = self.full_bak > + else: > + parent = self.bitmaps[-1].last_target() Is this intentional or should this be bitmap.last_target()? In this patch, no caller of create_incremental() gives any parameter, so there's no difference. > + > + target = bitmap.new_target(num) > + self.img_create(target, iotests.imgfmt, parent=parent) > + > + result = self.vm.qmp('drive-backup', device=bitmap.node, > + sync='dirty-bitmap', bitmap=bitmap.name, > + format=iotests.imgfmt, target=target, > + mode='existing') > + self.assert_qmp(result, 'return', {}) > + > + event = self.wait_until_completed(bitmap.node, check_offset=validate) > + if validate: > + return self.check_incremental(target) > + > + > + def check_incremental(self, target=None): > + if target is None: > + target = self.bitmaps[-1].last_target() > + self.assertTrue(iotests.compare_images(self.test_img, target)) > + return True > + > + > + def hmp_io_writes(self, drive, patterns): > + for pattern in patterns: > + self.vm.hmp_qemu_io(drive, 'write -P%s %s %s' % pattern) > + self.vm.hmp_qemu_io(drive, 'flush') > + > + > + # Create a bitmap and full backup before VM execution begins, > + # then create a series of incremental backups during execution. > + def test_incremental_simple(self): > + self.create_full_backup() > + self.add_bitmap('bitmap0', 'drive0') > + self.vm.hmp('c') self.vm.qmp('continue') works, too, but as I said above, I think accel=qtest should be enough so you may not need this at all. (I'm saying this because we all know that QMP > HMP (obviously)) So I had some small questions which I think I want to have answers before giving an R-b, but the test logic looks good to me. Max > + # Sanity: Create a "hollow" incremental backup > + self.create_incremental() > + # Three writes: One complete overwrite, one new segment, > + # and one partial overlap. > + self.hmp_io_writes('drive0', (('0xab', 0, 512), > + ('0xfe', '16M', '256k'), > + ('0x64', '32736k', '64k'))) > + self.create_incremental() > + # Three more writes, one of each kind, like above > + self.hmp_io_writes('drive0', (('0x9a', 0, 512), > + ('0x55', '8M', '352k'), > + ('0x78', '15872k', '1M'))) > + self.create_incremental() > + return True > + > + > def test_sync_dirty_bitmap_missing(self): > self.assert_no_active_block_jobs() > self.files.append(self.foo_img) > diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out > index fbc63e6..8d7e996 100644 > --- a/tests/qemu-iotests/112.out > +++ b/tests/qemu-iotests/112.out > @@ -1,5 +1,5 @@ > -.. > +... > ---------------------------------------------------------------------- > -Ran 2 tests > +Ran 3 tests > > OK > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py > index 241b5ee..6bff935 100644 > --- a/tests/qemu-iotests/iotests.py > +++ b/tests/qemu-iotests/iotests.py > @@ -88,6 +88,11 @@ class VM(object): > '-display', 'none', '-vga', 'none'] > self._num_drives = 0 > > + # Add arbitrary arguments to the command-line. > + def add_args(self, arglist): > + self._args = self._args + arglist > + return self > + > # This can be used to add an unused monitor instance. > def add_monitor_telnet(self, ip, port): > args = 'tcp:%s:%d,server,nowait,telnet' % (ip, port) > @@ -109,23 +114,24 @@ class VM(object): > self._num_drives += 1 > return self > > + def hmp(self, args): > + return self.qmp('human-monitor-command', > + command_line=args) > + > def pause_drive(self, drive, event=None): > '''Pause drive r/w operations''' > if not event: > self.pause_drive(drive, "read_aio") > self.pause_drive(drive, "write_aio") > return > - self.qmp('human-monitor-command', > - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive)) > + self.hmp('qemu-io %s "break %s bp_%s"' % (drive, event, drive)) > > def resume_drive(self, drive): > - self.qmp('human-monitor-command', > - command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive)) > + self.hmp('qemu-io %s "remove_break bp_%s"' % (drive, drive)) > > def hmp_qemu_io(self, drive, cmd): > '''Write to a given drive using an HMP command''' > - return self.qmp('human-monitor-command', > - command_line='qemu-io %s "%s"' % (drive, cmd)) > + return self.hmp('qemu-io %s "%s"' % (drive, cmd)) > > def add_fd(self, fd, fdset, opaque, opts=''): > '''Pass a file descriptor to the VM'''