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 <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

Reply via email to