Change in vdsm[master]: [WIP] core: Expose API for qemu-img commit

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: [WIP] core: Expose API for qemu-img commit
......................................................................


Patch Set 2:

(6 comments)

Great work!

https://gerrit.ovirt.org/#/c/64222/2//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-09-20 20:30:58 +0300
Line 4: Commit:     Ala Hino <ahino(a)redhat.com&gt;
Line 5: CommitDate: 2016-09-20 20:36:48 +0300
Line 6: 
Line 7: [WIP] core: Expose API for qemu-img commit
Use qemuimg: ...
Line 8: 
Line 9: Change-Id: If7a13be40541fb268541bd8614a642263b96b487


https://gerrit.ovirt.org/#/c/64222/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 185:     return QemuImgOperation(cmd, cwd=cwdPath)
Line 186: 
Line 187: 
Line 188: def commit(top, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
We should also use -t none like we use in convert, and if possible, also -T none. This use
directio, avoiding spamming the page cache with data from the image, and also safer since
the page cache may not be correct if another host was writing to the same image before.
Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")


Line 188: def commit(top, base=None):
Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Please explain why -d is needed, the code is very clear about adding it when base is not
specified, but we need to know why. I see the patch - "removed unneeded -d option
(works for me)".
Line 193:         cmd.append("-d")
Line 194: 
Line 195:     return QemuImgOperation(cmd)
Line 196: 


Line 189:     cmd = [_qemuimg.cmd, "commit", "-p"]
Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")
We must use -f to enforce the format in the metadata. Letting qemu detect the format is a
security issue.
Line 194: 
Line 195:     return QemuImgOperation(cmd)
Line 196: 
Line 197: 


Line 190:     if base:
Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")
Line 194: 
Appending top the command will increase the chance for successful commit.
Line 195:     return QemuImgOperation(cmd)
Line 196: 
Line 197: 
Line 198: class QemuImgOperation(object):


Line 191:         cmd.extend(("-b", base))
Line 192:     else:
Line 193:         cmd.append("-d")
Line 194: 
Line 195:     return QemuImgOperation(cmd)
I think we must change the working directory to the image directory as we do in convert()
- we need the same logic, maybe we should move the code to a little helper (in another
patch before this one).
Line 196: 
Line 197: 
Line 198: class QemuImgOperation(object):
Line 199:     REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')


-- 
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org&gt;
Gerrit-HasComments: Yes