Maor Lipchuk has posted comments on this change.

Change subject: [QCOW1.1]qemuimg: Add kwargs optional parameter for qcow.
......................................................................


Patch Set 4:

(6 comments)

@Nir, I disagree regarding the approach of no upload new versions while the 
tests are broken since this patch is a WIP and posted as a draft.
Once the tests will be fixed I will publish this draft patch

https://gerrit.ovirt.org/#/c/64169/4//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2016-09-20 01:41:01 +0300
Line 4: Commit:     Maor Lipchuk <mlipc...@redhat.com>
Line 5: CommitDate: 2016-09-20 03:56:54 +0300
Line 6: 
Line 7: [QCOW1.1]qemuimg: Add kwargs optional parameter for qcow.
> We don't use such titles ([FOO]).
I'm not sure why to use qcow3?
qcow_compat 1.1 should be supported for qcow2 as well...
I would simply use qemuimg without QCOW1.1

The Version attribute in the qcow header indicates the qcow version (qcow1, 
qcow2, qcow3), it should not be related to  the compat version AFAIK
Line 8: 
Line 9: Add kwargs of storage domain metadata to indicate the
Line 10: appropriate qcow level to use for create and convert operations.
Line 11: 


https://gerrit.ovirt.org/#/c/64169/4/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 45:     RAW = "raw"
Line 46:     VMDK = "vmdk"
Line 47: 
Line 48: _QCOW2_COMPAT_SUPPORTED = ("0.10", "1.1")
Line 49: _QCOW2_1_1 = "1.1"
> We need a better name for this, and we need another constant for 0.10, and 
Do you have any suggestions for names?

How about
_QCOW_COMPAT_LEVEL_0_10 and _QCOW_COMPAT_LEVEL_1_1 ?
Line 50: 
Line 51: 
Line 52: def supports_compat(compat):
Line 53:     return compat in _QCOW2_COMPAT_SUPPORTED


Line 102:     return info
Line 103: 
Line 104: 
Line 105: def create(image, size=None, format=None, backing=None,
Line 106:            backingFormat=None, **kwargs):
> Please replace this change with qcow2_compat=None.
Why not simply call it sd_version? qcow2_compat should be  derived from the 
sd_version
Line 107:     cmd = [_qemuimg.cmd, "create"]
Line 108:     cwdPath = None
Line 109: 
Line 110:     if format:


Line 154:         raise QImgError(rc, out, err, "unable to parse qemu-img check 
output")
Line 155: 
Line 156: 
Line 157: def convert(srcImage, dstImage, srcFormat=None, dstFormat=None,
Line 158:             backing=None, backingFormat=None, **kwargs):
> Same, replace with qcow_compat=None
see previous comment
Line 159:     cmd = [_qemuimg.cmd, "convert", "-p", "-t", "none", "-T", "none"]
Line 160:     options = []
Line 161:     cwdPath = None
Line 162: 


Line 307:     if rc != 0:
Line 308:         raise QImgError(rc, out, err)
Line 309: 
Line 310: 
Line 311: def _qcow2_compat(**kwargs):
> Please rename to _default_qcow2_compat()
done, I assume we should also change this to pass only the sd version?
Line 312:     sd_version = kwargs.get('version')
Line 313:     if sd_version == '4':
Line 314:         return _QCOW2_1_1
Line 315:     value = config.get('irs', 'qcow2_compat')


Line 309: 
Line 310: 
Line 311: def _qcow2_compat(**kwargs):
Line 312:     sd_version = kwargs.get('version')
Line 313:     if sd_version == '4':
> >= 4 perhaps?
indeed, done
Line 314:         return _QCOW2_1_1
Line 315:     value = config.get('irs', 'qcow2_compat')
Line 316:     if value not in _QCOW2_COMPAT_SUPPORTED:
Line 317:         raise exception.InvalidConfiguration(


-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Adam Litke <ali...@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