Change in vdsm[master]: vm: Unify checks for vdsm image

nsoffer at redhat.com nsoffer at redhat.com
Sat Dec 14 02:50:42 UTC 2013


Nir Soffer has posted comments on this change.

Change subject: vm: Unify checks for vdsm image
......................................................................


Patch Set 2:

(1 comment)

....................................................
File vdsm/vm.py
Line 88:     :type drive: dict or vm.Drive
Line 89:     :return: bool
Line 90:     """
Line 91:     if drive['device'] != 'disk':
Line 92:         return False
I'm not arguing if the device == 'disk' check is needed or not - this patch is not about removing unneeded checks but about removing code duplication, make the code simpler to understand and easier to use.

What you are trying to do in http://gerrit.ovirt.org/22363, remove one disk check, make the code worse, adding more places with different logic. We should first unify the checks, removing the code duplication. Then, we can remove unneeded checks in one place.
Line 93:     required = ('volumeID', 'domainID', 'imageID', 'poolID')
Line 94:     return all(k in drive for k in required)
Line 95: 
Line 96: 


-- 
To view, visit http://gerrit.ovirt.org/22370
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa109a19aeec56f09ed2e6d64dee636c7779d426
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list