Bala.FA has posted comments on this change. Change subject: Provide device details using python-blivet module ......................................................................
Patch Set 4: Code-Review-1 (11 comments) http://gerrit.ovirt.org/#/c/35028/4/vdsm/gluster/storagedev.py File vdsm/gluster/storagedev.py: Line 20: Line 21: import blivet Line 22: from . import makePublic Line 23: Line 24: _bb = None 1. You could user better name like _blivet_env ? 2. Does this object reflect newly added devices dynamically? Line 25: Line 26: Line 27: def _getBlivet(): Line 28: global _bb Line 23: Line 24: _bb = None Line 25: Line 26: Line 27: def _getBlivet(): If you change global name, change this function name accordingly. Line 28: global _bb Line 29: if _bb: Line 30: return _bb Line 31: Line 30: return _bb Line 31: Line 32: _bb = blivet.Blivet() Line 33: _bb.reset() Line 34: return _bb This could be done even simpler/cleaner like if not _bb: ... ... return _bb Line 35: Line 36: Line 37: @makePublic Line 38: def hostDeviceList(devType): Line 34: return _bb Line 35: Line 36: Line 37: @makePublic Line 38: def hostDeviceList(devType): Please change this name to storageDeviceList which gives more meaning Line 39: b = _getBlivet() Line 40: deviceInfo = [] Line 41: for device in b.devices: Line 42: if devType != '': Line 38: def hostDeviceList(devType): Line 39: b = _getBlivet() Line 40: deviceInfo = [] Line 41: for device in b.devices: Line 42: if devType != '': 1. you should use if devType: ... 2. even simpler if does here if devType and devType != device.type: ... Line 43: if devType != device.type: Line 44: continue Line 45: info = {} Line 46: info['name'] = device.name Line 39: b = _getBlivet() Line 40: deviceInfo = [] Line 41: for device in b.devices: Line 42: if devType != '': Line 43: if devType != device.type: What are the possible device types? I would suggest to have enum for this than expecting something Line 44: continue Line 45: info = {} Line 46: info['name'] = device.name Line 47: if device.parents: Line 45: info = {} Line 46: info['name'] = device.name Line 47: if device.parents: Line 48: info['parent'] = device.parents[0].name Line 49: info['kids'] = device.kids What does device.kids have? If they are available as b.devices, why do we need to populate here? Line 50: if hasattr(device.format, 'mountpoint'): Line 51: info['mount'] = device.format.mountpoint Line 52: info['mountopts'] = device.format.mountopts Line 53: info['fileSystem'] = device.format.type Line 49: info['kids'] = device.kids Line 50: if hasattr(device.format, 'mountpoint'): Line 51: info['mount'] = device.format.mountpoint Line 52: info['mountopts'] = device.format.mountopts Line 53: info['fileSystem'] = device.format.type I believe you should send empty values when device.format is missing to engine to work correctly Line 54: info['model'] = device.model Line 55: info['type'] = device.type Line 56: if device.format.uuid: Line 57: info['uuid'] = device.format.uuid Line 53: info['fileSystem'] = device.format.type Line 54: info['model'] = device.model Line 55: info['type'] = device.type Line 56: if device.format.uuid: Line 57: info['uuid'] = device.format.uuid same here Line 58: if device.uuid: Line 59: info['devUuid'] = device.uuid Line 60: info['size'] = device.size Line 61: deviceInfo.append(info) Line 55: info['type'] = device.type Line 56: if device.format.uuid: Line 57: info['uuid'] = device.format.uuid Line 58: if device.uuid: Line 59: info['devUuid'] = device.uuid same here Line 60: info['size'] = device.size Line 61: deviceInfo.append(info) Line 56: if device.format.uuid: Line 57: info['uuid'] = device.format.uuid Line 58: if device.uuid: Line 59: info['devUuid'] = device.uuid Line 60: info['size'] = device.size what is the unit of the size and what is expected unit of the size? Line 61: deviceInfo.append(info) -- To view, visit http://gerrit.ovirt.org/35028 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85f520c6f476731cb5982d8256cb701387be87cf Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Timothy Asir <[email protected]> Gerrit-Reviewer: Bala.FA <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Nishanth Thomas <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Timothy Asir <[email protected]> Gerrit-Reviewer: [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
