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 <ahino(a)redhat.com>
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes