Nir Soffer has posted comments on this change. Change subject: clientIF: replace type() with isinstance() ......................................................................
Patch Set 1: Code-Review-1 (3 comments) This does not fix anything. .................................................... Commit Message Line 6: Line 7: clientIF: replace type() with isinstance() Line 8: Line 9: the very first line of prepareVolumePath is a type() check. Line 10: this is not very pythonic and, most important, the remainder This is not "not very pythonic" Line 11: of the code doesn't have evidences of a such a strict requirement. Line 12: Line 13: this patch replaces the type() check with a more pythonic Line 14: and extendible isinstance() check. Line 9: the very first line of prepareVolumePath is a type() check. Line 10: this is not very pythonic and, most important, the remainder Line 11: of the code doesn't have evidences of a such a strict requirement. Line 12: Line 13: this patch replaces the type() check with a more pythonic This is not "more pythonic" Line 14: and extendible isinstance() check. Line 15: Line 16: http://effbot.org/pyfaq/how-do-i-check-if-an-object-is-an-instance-of-a-given-class-or-of-a-subclass-of-it.htm Line 17: .................................................... File vdsm/clientIF.py Line 233: self.log.info('Error finding path for device', exc_info=True) Line 234: raise vm.VolumeError(uuid) Line 235: Line 236: def prepareVolumePath(self, drive, vmId=None): Line 237: if isinstance(drive, dict): This is little better then type, because now you can send here a dict subclass. But this fix is just as bad as the type check. For example, if I send this object, the check will fail but it should not: class PDIV(object): device = 'disk' def __init__(self, poolID, domainID, imageID, volumeID): self.poolID = poolID self.domainID = domainID self.imageID = imageID self.volumeID = volumeID def __contains__(self, name): return hasattr(self, name) def __getitem__(self, name): try: return getattr(self, name) except AttributeError: raise KeyError(name) def __setitem__(self, name, value): setattr(self, name, value) This method accept None, str, and dict arguments. These types are not related and we should not accept them. The real fix is removing the support for None and str and let this method explode when you try to use drive as a dict, and it does not implement a the required interface. This requires fixing of the caller that call this with None and str. Line 238: # PDIV drive format Line 239: if drive['device'] == 'disk' and vm.isVdsmImage(drive): Line 240: res = self.irs.prepareImage( Line 241: drive['domainID'], drive['poolID'], -- To view, visit http://gerrit.ovirt.org/23056 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic53afef3476a2aedbc5dac830a99f6ee9c093688 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
