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

Reply via email to