Nir Soffer has posted comments on this change.
Change subject: core: Expose API for qemuimg commit ......................................................................
Patch Set 11:
(15 comments)
https://gerrit.ovirt.org/#/c/64222/11/tests/qemuimg_test.py File tests/qemuimg_test.py:
Line 368: @expandPermutations Line 369: class TestCommit(TestCaseBase): Line 370: Line 371: @permutations([ Line 372: # base_format, qcow2_compat, base, top, use_base Lets add comments explaining the use case:
# Merging internal volume Line 373: (qemuimg.FORMAT.RAW, "1.1", 0, 1, False), Line 374: (qemuimg.FORMAT.RAW, "1.1", 0, 1, True), Line 375: (qemuimg.FORMAT.RAW, "0.10", 0, 1, False), Line 376: (qemuimg.FORMAT.RAW, "0.10", 0, 1, True),
Line 372: # base_format, qcow2_compat, base, top, use_base Line 373: (qemuimg.FORMAT.RAW, "1.1", 0, 1, False), Line 374: (qemuimg.FORMAT.RAW, "1.1", 0, 1, True), Line 375: (qemuimg.FORMAT.RAW, "0.10", 0, 1, False), Line 376: (qemuimg.FORMAT.RAW, "0.10", 0, 1, True), Add comment - seems like:
# Merging entire chain into the base Line 377: (qemuimg.FORMAT.RAW, "1.1", 0, 4, True), Line 378: (qemuimg.FORMAT.RAW, "0.10", 0, 4, True), Line 379: (qemuimg.FORMAT.RAW, "1.1", 1, 2, True), Line 380: (qemuimg.FORMAT.RAW, "0.10", 1, 2, True),
Line 374: (qemuimg.FORMAT.RAW, "1.1", 0, 1, True), Line 375: (qemuimg.FORMAT.RAW, "0.10", 0, 1, False), Line 376: (qemuimg.FORMAT.RAW, "0.10", 0, 1, True), Line 377: (qemuimg.FORMAT.RAW, "1.1", 0, 4, True), Line 378: (qemuimg.FORMAT.RAW, "0.10", 0, 4, True), Comment:
# Merging internal volume into its parent volume in cow format Line 379: (qemuimg.FORMAT.RAW, "1.1", 1, 2, True), Line 380: (qemuimg.FORMAT.RAW, "0.10", 1, 2, True), Line 381: (qemuimg.FORMAT.RAW, "1.1", 3, 4, True), Line 382: (qemuimg.FORMAT.RAW, "0.10", 3, 4, True),
Line 376: (qemuimg.FORMAT.RAW, "0.10", 0, 1, True), Line 377: (qemuimg.FORMAT.RAW, "1.1", 0, 4, True), Line 378: (qemuimg.FORMAT.RAW, "0.10", 0, 4, True), Line 379: (qemuimg.FORMAT.RAW, "1.1", 1, 2, True), Line 380: (qemuimg.FORMAT.RAW, "0.10", 1, 2, True), This looks like the previous case (base=1, top=2) - do we need these permutations? Line 381: (qemuimg.FORMAT.RAW, "1.1", 3, 4, True), Line 382: (qemuimg.FORMAT.RAW, "0.10", 3, 4, True), Line 383: (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True), Line 384: (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True),
Line 378: (qemuimg.FORMAT.RAW, "0.10", 0, 4, True), Line 379: (qemuimg.FORMAT.RAW, "1.1", 1, 2, True), Line 380: (qemuimg.FORMAT.RAW, "0.10", 1, 2, True), Line 381: (qemuimg.FORMAT.RAW, "1.1", 3, 4, True), Line 382: (qemuimg.FORMAT.RAW, "0.10", 3, 4, True), This is different since we merge a subchain. Line 383: (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True), Line 384: (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True), Line 385: ]) Line 386: def test_commit(self, base_format, qcow2_compat, base, top,
Line 380: (qemuimg.FORMAT.RAW, "0.10", 1, 2, True), Line 381: (qemuimg.FORMAT.RAW, "1.1", 3, 4, True), Line 382: (qemuimg.FORMAT.RAW, "0.10", 3, 4, True), Line 383: (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True), Line 384: (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True), Do we really care about the case when the first volume is not raw? we can simplify the tests by using always raw format for volume 0. Line 385: ]) Line 386: def test_commit(self, base_format, qcow2_compat, base, top, Line 387: use_base=True): Line 388: size = 1048576
Line 382: (qemuimg.FORMAT.RAW, "0.10", 3, 4, True), Line 383: (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True), Line 384: (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True), Line 385: ]) Line 386: def test_commit(self, base_format, qcow2_compat, base, top, We cannot call this base_format when base is some volume in the middle. We need another name. Line 387: use_base=True): Line 388: size = 1048576 Line 389: with namedTemporaryDir() as tmpdir: Line 390: chain = []
Line 383: (qemuimg.FORMAT.QCOW2, "1.1", 1, 3, True), Line 384: (qemuimg.FORMAT.QCOW2, "0.10", 1, 3, True), Line 385: ]) Line 386: def test_commit(self, base_format, qcow2_compat, base, top, Line 387: use_base=True): We don't need defaults, the permutations set the value. Line 388: size = 1048576 Line 389: with namedTemporaryDir() as tmpdir: Line 390: chain = [] Line 391: parent = None
Line 388: size = 1048576 Line 389: with namedTemporaryDir() as tmpdir: Line 390: chain = [] Line 391: parent = None Line 392: # Create a chain of 5 volumes. Better use only 4 volumes, will make the tests little faster. Line 393: for i in range(0, 5): Line 394: vol = os.path.join(tmpdir, "vol%d.img" % i) Line 395: format = base_format if i == 0 else qemuimg.FORMAT.QCOW2 Line 396: make_image(vol, size, format, i, qcow2_compat, parent)
Line 389: with namedTemporaryDir() as tmpdir: Line 390: chain = [] Line 391: parent = None Line 392: # Create a chain of 5 volumes. Line 393: for i in range(0, 5): range(0, 5) is equal to range(5), and more idomatic. Line 394: vol = os.path.join(tmpdir, "vol%d.img" % i) Line 395: format = base_format if i == 0 else qemuimg.FORMAT.QCOW2 Line 396: make_image(vol, size, format, i, qcow2_compat, parent) Line 397: blocks = os.stat(vol).st_blocks
Line 402: topFormat=qemuimg.FORMAT.QCOW2, Line 403: base=chain[base][0] if use_base else None) Line 404: op.wait_for_completion() Line 405: Line 406: for i in range(base, top): Nice!
But it will never return top - we need range(base, top + 1) Line 407: offset = "{}k".format(i) Line 408: pattern = 0xf0 + i Line 409: # The base volume must have the data from all the volumes Line 410: # merged into it.
Line 407: offset = "{}k".format(i) Line 408: pattern = 0xf0 + i Line 409: # The base volume must have the data from all the volumes Line 410: # merged into it. Line 411: format = base_format if i == 0 else qemuimg.FORMAT.QCOW2 Removing base_format and using always raw would make this simpler. Line 412: qemu_pattern_verify(chain[base][0], format, offset=offset, Line 413: len='1k', pattern=pattern) Line 414: if i > 0: Line 415: # internal and top volumes should keep the data, we
Line 408: pattern = 0xf0 + i Line 409: # The base volume must have the data from all the volumes Line 410: # merged into it. Line 411: format = base_format if i == 0 else qemuimg.FORMAT.QCOW2 Line 412: qemu_pattern_verify(chain[base][0], format, offset=offset, chain[base][0] makes the code harder to follow. Since base is constant, we can do this before the loop:
base_vol = chain[base][0]
And use base_vol here. Line 413: len='1k', pattern=pattern) Line 414: if i > 0: Line 415: # internal and top volumes should keep the data, we Line 416: # may want to wipe this data when deleting the volumes
Line 410: # merged into it. Line 411: format = base_format if i == 0 else qemuimg.FORMAT.QCOW2 Line 412: qemu_pattern_verify(chain[base][0], format, offset=offset, Line 413: len='1k', pattern=pattern) Line 414: if i > 0: Should be i > base. Line 415: # internal and top volumes should keep the data, we Line 416: # may want to wipe this data when deleting the volumes Line 417: # later. Line 418: self.assertEqual(os.stat(chain[i][0]).st_blocks,
Line 415: # internal and top volumes should keep the data, we Line 416: # may want to wipe this data when deleting the volumes Line 417: # later. Line 418: self.assertEqual(os.stat(chain[i][0]).st_blocks, Line 419: chain[i][1]) This should work, but we can be more clear.
vol, blocks = chain[i] self.assertEqual(os.stat(vol).st_blocks, blocks) Line 420: Line 421: def test_commit_progress(self): Line 422: with namedTemporaryDir() as tmpdir: Line 423: size = 1048576