Nir Soffer has posted comments on this change. Change subject: gluster: fix size conversion issues in brick create ......................................................................
Patch Set 6: Code-Review-1 (5 comments) https://gerrit.ovirt.org/#/c/50705/6/vdsm/gluster/storagedev.py File vdsm/gluster/storagedev.py: Line 65: Line 66: # This method helps to convert the size to given Unittype. Line 67: # This is required to since there a incompatible change in blivet API Line 68: # size.convertTo where older versions required string param but newer versions Line 69: # requires the constants in blivet.size.KiB. blivet.size.KiB -> blivet.size. Line 70: def _getDeviceSize(device, unitType='MiB'): Line 71: if hasattr(blivet.size, 'MiB'): Line 72: if unitType == 'KiB': Line 73: return device.size.convertTo(blivet.size.KiB) Line 66: # This method helps to convert the size to given Unittype. Line 67: # This is required to since there a incompatible change in blivet API Line 68: # size.convertTo where older versions required string param but newer versions Line 69: # requires the constants in blivet.size.KiB. Line 70: def _getDeviceSize(device, unitType='MiB'): Add a note that this part is for the new blivet version. Line 71: if hasattr(blivet.size, 'MiB'): Line 72: if unitType == 'KiB': Line 73: return device.size.convertTo(blivet.size.KiB) Line 74: elif unitType == 'MiB': Line 71: if hasattr(blivet.size, 'MiB'): Line 72: if unitType == 'KiB': Line 73: return device.size.convertTo(blivet.size.KiB) Line 74: elif unitType == 'MiB': Line 75: return device.size.convertTo(blivet.size.MiB) This should raise, the caller provided unitType we don't know how to handle. Line 76: else: Line 77: return device.size.convertTo(spec=unitType) Line 78: return 0 Line 79: Line 72: if unitType == 'KiB': Line 73: return device.size.convertTo(blivet.size.KiB) Line 74: elif unitType == 'MiB': Line 75: return device.size.convertTo(blivet.size.MiB) Line 76: else: Add a note that this is for old blivet version supporting spec string. Line 77: return device.size.convertTo(spec=unitType) Line 78: return 0 Line 79: Line 80: Line 74: elif unitType == 'MiB': Line 75: return device.size.convertTo(blivet.size.MiB) Line 76: else: Line 77: return device.size.convertTo(spec=unitType) Line 78: return 0 Not needed, and a bad way to fail, now all the callers have to check for 0, or we will fail much later when someone try to use the invalid size, making debugging much harder. I suggest to do this: if hasattr(blivet.size, 'MiB'): # New blivet requires size constants (e.g, size.MiB, size.KiB) unit = getattr(blivet.size, unitType) return device.size.convertTo(unit) else: # Old blivet using spec string return device.size.convertTo(spec=unitType) This will raise AttributeError if you call with invalid unitType, which should make this easy to debug. Line 79: Line 80: Line 81: def _getDeviceDict(device, createBrick=False): Line 82: info = {'name': device.name, -- To view, visit https://gerrit.ovirt.org/50705 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I576b1a8c880ef6f355157225c7b763378d8cf46d Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ramesh N <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
