Nir Soffer has posted comments on this change. Change subject: qemuimg: Add kwargs optional parameter for qcow. ......................................................................
Patch Set 6: (4 comments) https://gerrit.ovirt.org/#/c/64169/6/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 308: if rc != 0: Line 309: raise QImgError(rc, out, err) Line 310: Line 311: Line 312: def _qcow2_def_compat(): > @Nir, Eventually we will probably pass the qcow2_compat at every call for t It returns the default compat, which depend on the configuration since 4.0. We want to keep this behavior. Line 313: value = config.get('irs', 'qcow2_compat') Line 314: if value not in _QCOW2_COMPAT_SUPPORTED: Line 315: raise exception.InvalidConfiguration( Line 316: "Unsupported value for irs:qcow2_compat: %r" % value) https://gerrit.ovirt.org/#/c/64169/7/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 108: Line 109: if format: Line 110: cmd.extend(("-f", format)) Line 111: if format == FORMAT.QCOW2: Line 112: v_compat = qcow2_compat if qcow2_compat else qcow2_def_compat() > The default is None so in that case you will be creating an invalid command We need to support the qcow configuration introduced in 4.0. if qcow2_compat is None: qcow2_compat = default_qcow2_compat() I agree with Adam that we should raise if the provided value is not supported. Please add tests for this option. The current tests are good enough for the default value - nothing should change, but when using the option, the test should verify that we create the command properly, regardless of the current config. Line 113: cmd.extend(('-o', 'compat=' + v_compat)) Line 114: Line 115: if backing: Line 116: if not os.path.isabs(backing): Line 166: cmd.append(srcImage) Line 167: Line 168: if dstFormat: Line 169: cmd.extend(("-O", dstFormat)) Line 170: if dstFormat == FORMAT.QCOW2: Same as create. Line 171: v_compat = qcow2_compat if qcow2_compat else qcow2_def_compat() Line 172: options.append('compat=' + v_compat) Line 173: Line 174: if backing: Line 306: ioclass=utils.IOCLASS.IDLE) Line 307: Line 308: if rc != 0: Line 309: raise QImgError(rc, out, err) Line 310: Please rename to default_qcow2_compat Line 311: Line 312: def _qcow2_def_compat(): Line 313: value = config.get('irs', 'qcow2_compat') Line 314: if value not in _QCOW2_COMPAT_SUPPORTED: -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- [email protected] To unsubscribe send an email to [email protected]
