Francesco Romani has posted comments on this change. Change subject: qemuimg: Handle new output format ......................................................................
Patch Set 6: Code-Review+1 (5 comments) seems OK and nicely tested. http://gerrit.ovirt.org/#/c/27552/6/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 86: 'virtualsize': int(__iregexSearch("virtualsize", out[2])), Line 87: } Line 88: Line 89: # Scan for optional fields in the output Line 90: row = 4 Not a fan of magic numbers, but that was also in the old code - you actually removed one of them - so good enough for now. Can be cleaned further on a later patch. Line 91: for field, filterFn in (('clustersize', int), ('backingfile', str)): Line 92: try: Line 93: info[field] = filterFn(__iregexSearch(field, out[row])) Line 94: row = row + 1 Line 94: row = row + 1 Line 95: except (_RegexSearchError, IndexError): Line 96: pass Line 97: except _RegexSearchError: Line 98: raise QImgError(rc, out, err, "unable to parse qemu-img info output") what about narrowing down this try/except clause by moving the except to line 88? In a later patch of course Line 99: Line 100: return info Line 101: Line 102: http://gerrit.ovirt.org/#/c/27552/6/tests/qemuimgTests.py File tests/qemuimgTests.py: Line 90: Line 91: @monkeypatch.MonkeyPatch(utils, 'execCmd', Line 92: partial(fakeCmd, outputQemu1NoBackingFile)) Line 93: def testQemu1NoBackingFile(self): Line 94: info = qemuimg.info('foo') I'd rather use 'unused' or something like it, but this is very minor, don't bother to change unless you need to resubmit. Line 95: self.assertFalse('backingfile' in info) Line 96: Line 97: @monkeypatch.MonkeyPatch(utils, 'execCmd', Line 98: partial(fakeCmd, outputQemu1Backing)) Line 91: @monkeypatch.MonkeyPatch(utils, 'execCmd', Line 92: partial(fakeCmd, outputQemu1NoBackingFile)) Line 93: def testQemu1NoBackingFile(self): Line 94: info = qemuimg.info('foo') Line 95: self.assertFalse('backingfile' in info) assertNotIn was backported, we can make use of it. Again, don't bother to change unless you need to resubmit. Line 96: Line 97: @monkeypatch.MonkeyPatch(utils, 'execCmd', Line 98: partial(fakeCmd, outputQemu1Backing)) Line 99: def testQemu1Backing(self): Line 106: info = qemuimg.info('foo') Line 107: self.assertEquals('qcow2', info['format']) Line 108: self.assertEquals(1073741824, info['virtualsize']) Line 109: self.assertEquals(65536, info['clustersize']) Line 110: self.assertFalse('backingfile' in info) I believe it would be better to do like we do in vmTestsData. The test data (for us, domain XML) is a template, and we have pairs of template data, config data. This way we do not duplicate the actual value to be checked. But this is very minor so it will surely wait future patches. Line 111: Line 112: @monkeypatch.MonkeyPatch(utils, 'execCmd', Line 113: partial(fakeCmd, outputQemu2BackingNoCluster)) Line 114: def testQemu2BackingNoCluster(self): -- To view, visit http://gerrit.ovirt.org/27552 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic229198ab7c2bb9743bdf8629416131186115431 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
