From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39600) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLfN5-0002KJ-S5 for qemu-devel@nongnu.org; Wed, 11 Feb 2015 17:03:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLfN2-0003dG-Kx for qemu-devel@nongnu.org; Wed, 11 Feb 2015 17:02:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41047) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLfN2-0003d6-ED for qemu-devel@nongnu.org; Wed, 11 Feb 2015 17:02:56 -0500 Message-ID: <54DBD18E.10703@redhat.com> Date: Wed, 11 Feb 2015 17:02:54 -0500 From: John Snow MIME-Version: 1.0 References: <1423532117-14490-1-git-send-email-jsnow@redhat.com> <1423532117-14490-15-git-send-email-jsnow@redhat.com> <54DBCC59.7060303@redhat.com> In-Reply-To: <54DBCC59.7060303@redhat.com> Content-Type: text/plain; charset=utf-8; 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: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 02/11/2015 04:40 PM, Max Reitz wrote: > 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? > Maybe not? It's baggage from copying 056 as a template. >> 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. > It's probably more semantically appropriate, yes. >> + 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... > Oh, good point. > 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? Yes, this is actually somewhat vestigial from a standalone version of this that would tolerate an arbitrary image. I left it in because I thought it was harmless and it served as an illustration about the inferred timing of the commands being tested. That said, it /can/ be removed. Its only purpose is illustrative. > >> 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? > Just having a really good time in python. I think "is" tests instances; it's like a strict equals. == is equivalence as you and I are used to it. In this case, it winds up being the same because python has some numerical objects cached, and any equation that evaluates to 0 evaluates to the /same/ 0. == is more correct, though, apparently. >> + 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. > Vestigial from the standalone edition. 'bitmap' should be preferred over bitmaps[-1], yes. >> + >> + 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 > Explanation above covers this; a more human-focused demi-interactive version preceded this iotest. I'll excise it and just use comments to create the same documentation effect. >> + # 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''' >