Ala Hino has posted comments on this change. Change subject: core: Expose API for qemuimg commit ......................................................................
Patch Set 5: (7 comments) Nir, note that I created a single test that covers all use cases. I am hoping that I addressed all your comments https://gerrit.ovirt.org/#/c/64222/2/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: 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. Line 191: workdir = os.path.dirname(top) Line 192: > Please explain why -d is needed, the code is very clear about adding it whe Done Line 193: if base: Line 194: cmd.extend(("-b", base)) Line 195: else: Line 196: # If base volume is not provided, qemuimg commit will empty the top https://gerrit.ovirt.org/#/c/64222/4/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 184: Line 185: return QemuImgOperation(cmd, cwd=cwdPath) Line 186: Line 187: Line 188: def commit(top, topFormat, base=None): > We should require topFormat, more secure to never let qemu detect the forma Done Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"] Line 190: # For simplicity, we always run commit in the working directory. Line 191: workdir = os.path.dirname(top) Line 192: Line 185: return QemuImgOperation(cmd, cwd=cwdPath) Line 186: Line 187: Line 188: def commit(top, topFormat, base=None): Line 189: cmd = [_qemuimg.cmd, "commit", "-p", "-t", "none"] > Add -t none. Done Line 190: # For simplicity, we always run commit in the working directory. Line 191: workdir = os.path.dirname(top) Line 192: Line 193: if base: 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. > workdir? Done Line 191: workdir = os.path.dirname(top) Line 192: Line 193: if base: Line 194: cmd.extend(("-b", base)) Line 191: workdir = os.path.dirname(top) Line 192: Line 193: if base: Line 194: cmd.extend(("-b", base)) Line 195: else: > Lets always use workdir, to simplify the code. Done 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 201: cmd.append("-d") Line 202: Line 203: cmd.extend(("-f", topFormat)) Line 204: Line 205: cmd.append(top) > str not needed Done Line 206: Line 207: return QemuImgOperation(cmd, cwd=workdir) Line 208: Line 209: https://gerrit.ovirt.org/#/c/64222/4/tests/qemuimg_test.py File tests/qemuimg_test.py: 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 346: ]) > This is broken because 0xf1 is not the pattern but the length. Done Line 347: def test_commit(self, chainLen, baseFormat, useBacking=True): Line 348: size = 1048576 Line 349: with namedTemporaryDir() as tmpdir: Line 350: # create base -- To view, visit https://gerrit.ovirt.org/64222 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If7a13be40541fb268541bd8614a642263b96b487 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org