Nir Soffer has posted comments on this change. Change subject: qemuimg: Add qcow_compat optional parameter. ......................................................................
Patch Set 18: (3 comments) https://gerrit.ovirt.org/#/c/64169/18/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 187: Line 188: return QemuImgOperation(cmd, cwd=cwdPath) Line 189: Line 190: Line 191: def validate_qcow2_compat(qcow2_compat): I think this should be private helper (_validate_qcow2_compat), and it should be located at the bottom of the file, near _parse_qemu_json. "qcow2_compat" is too wordy here, why not use "value"? Line 192: if qcow2_compat is None: Line 193: qcow2_compat = default_qcow2_compat() Line 194: if qcow2_compat in _QCOW2_COMPAT_SUPPORTED: Line 195: return qcow2_compat Line 189: Line 190: Line 191: def validate_qcow2_compat(qcow2_compat): Line 192: if qcow2_compat is None: Line 193: qcow2_compat = default_qcow2_compat() We can and should return here, we don't need to validate the return value of default_qcow2_compat. Line 194: if qcow2_compat in _QCOW2_COMPAT_SUPPORTED: Line 195: return qcow2_compat Line 196: else: Line 197: raise ValueError("Invalid compat version '%r'" % qcow2_compat) Line 193: qcow2_compat = default_qcow2_compat() Line 194: if qcow2_compat in _QCOW2_COMPAT_SUPPORTED: Line 195: return qcow2_compat Line 196: else: Line 197: raise ValueError("Invalid compat version '%r'" % qcow2_compat) This code works but it does not follow the vdsm style, using the happy path idiom - first check for errors and raise, then handle the normal flow. if qcow2_compat not in _QCOW2_COMPAT_SUPPORTED: raise ... return qcow2_compat Line 198: Line 199: Line 200: class QemuImgOperation(object): Line 201: REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*') -- 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
