Ala Hino has posted comments on this change.
Change subject: core: Expose API for qemuimg commit ......................................................................
Patch Set 6:
(8 comments)
https://gerrit.ovirt.org/#/c/64222/6/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py:
Line 186: Line 187: Line 188: def commit(top, topFormat, base=None): Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"] Line 190: # For simplicity, we always run commit in the working directory.
in the image directory
Done Line 191: workdir = os.path.dirname(top) Line 192: Line 193: if base: Line 194: cmd.extend(("-b", base))
Line 196: # If base volume is not provided, qemuimg commit will empty the top Line 197: # volume after the operation has succeeded. Providing '-d' option Line 198: # will cause qemuimg commit not to empty the top volume. Note that Line 199: # if a backing chain is provided, i.e. a base volume is provided, Line 200: # '-d' is always implied.
See my comment about this comment in previous version.
Done Line 201: cmd.append("-d") Line 202: Line 203: cmd.extend(("-f", topFormat)) Line 204:
Line 202: Line 203: cmd.extend(("-f", topFormat)) Line 204: Line 205: cmd.append(top) Line 206:
We use workdir only here, so better create it just before we use it. Smalle
Done Line 207: return QemuImgOperation(cmd, cwd=workdir) Line 208: Line 209: Line 210: class QemuImgOperation(object):
https://gerrit.ovirt.org/#/c/64222/6/tests/qemuimg_test.py File tests/qemuimg_test.py:
Line 335: @expandPermutations Line 336: class TestCommit(TestCaseBase): Line 337: Line 338: @MonkeyPatch(qemuimg, 'config', CONFIG) Line 339: @permutations([
We need a comment here for the argument
Done Line 340: (1, qemuimg.FORMAT.RAW, False), Line 341: (1, qemuimg.FORMAT.RAW), Line 342: (1, qemuimg.FORMAT.QCOW2, False), Line 343: (1, qemuimg.FORMAT.QCOW2),
Line 337: Line 338: @MonkeyPatch(qemuimg, 'config', CONFIG) Line 339: @permutations([ Line 340: (1, qemuimg.FORMAT.RAW, False), Line 341: (1, qemuimg.FORMAT.RAW),
Please keep the same number of arguments to make the format consistent and
Done Line 342: (1, qemuimg.FORMAT.QCOW2, False), Line 343: (1, qemuimg.FORMAT.QCOW2), Line 344: (3, qemuimg.FORMAT.RAW), Line 345: (3, qemuimg.FORMAT.QCOW2)
Line 343: (1, qemuimg.FORMAT.QCOW2), Line 344: (3, qemuimg.FORMAT.RAW), Line 345: (3, qemuimg.FORMAT.QCOW2) Line 346: ]) Line 347: def test_commit(self, chainLen, baseFormat, useBacking=True):
This looks like use_base, or with_base. backing is another term not related
Done Line 348: size = 1048576 Line 349: with namedTemporaryDir() as tmpdir: Line 350: # create base Line 351: base = os.path.join(tmpdir, "base.img")
Line 361: chain.append(vol) Line 362: parent = vol Line 363: Line 364: if useBacking: Line 365: op = qemuimg.commit(chain[chainLen-1],
You can use:
Done Line 366: topFormat=topFormat, base=base) Line 367: else: Line 368: op = qemuimg.commit(chain[chainLen-1], topFormat) Line 369: op.wait_for_completion()
Line 364: if useBacking: Line 365: op = qemuimg.commit(chain[chainLen-1], Line 366: topFormat=topFormat, base=base) Line 367: else: Line 368: op = qemuimg.commit(chain[chainLen-1], topFormat)
This is little ugly, can we have the same call, taking None as default valu
Done Line 369: op.wait_for_completion() Line 370: Line 371: # verify base data at offset 0 after commit Line 372: qemu_pattern_verify(base, baseFormat, offset='0', len='1k',