Nir Soffer has posted comments on this change. Change subject: qemuimg: Add qcow_compat optional parameter. ......................................................................
Patch Set 18: (2 comments) https://gerrit.ovirt.org/#/c/64169/18/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 101: return info Line 102: Line 103: Line 104: def create(image, size=None, format=None, backing=None, Line 105: backingFormat=None, qcow2_compat=None): This option is very clear here - format and qcow2_compat refer to the image being created. I also like the qcow2_compat style, but the old code is already using mixedCase, and we should match this style in the arguments. Also the new argument refer to the format argument, not to the backing, so the argument should come after format=None. Adding a parameter in the middle may break code calling this function incorrectly with positional arguments - this is good, we should fix such code. Another option is to switch this all arguments to lower_case style before adding this option - this will be a very nice cleanup if you can spend the time on this rename. If not, please match the arguments to the existing ugly and error prone style. Line 106: cmd = [_qemuimg.cmd, "create"] Line 107: cwdPath = None Line 108: Line 109: if format: Line 168: if dstFormat: Line 169: cmd.extend(("-O", dstFormat)) Line 170: if dstFormat == FORMAT.QCOW2: Line 171: qcow2_compat = validate_qcow2_compat(qcow2_compat) Line 172: options.append('compat=' + qcow2_compat) But here he compat refer to the destination format (dstFormat), so it should be named dst_qcow2_compat, and the parameter should appear after dstFormat, and not after backingFormat. Line 173: Line 174: if backing: Line 175: if not os.path.isabs(backing): Line 176: cwdPath = os.path.dirname(srcImage) -- To view, visit https://gerrit.ovirt.org/64169 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8bbf8a60d0af1f99b3fae2c30ac06b36d5986180 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Kaul <yk...@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
