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 <ah...@redhat.com>
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 <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