Change in vdsm[master]: Provide device details using python-blivet module

tjeyasin at redhat.com tjeyasin at redhat.com
Tue Nov 18 10:24:24 UTC 2014


Timothy Asir has posted comments on this change.

Change subject: Provide device details using python-blivet module
......................................................................


Patch Set 4:

(10 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 ?
1. Done
2. No
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.
Done
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
Done
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
Done
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
Done
Line 43:             if devType != device.type:
Line 44:                 continue
Line 45:         info = {}
Line 46:         info['name'] = device.name


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
device.kids provides no of dependent count which is a numeric value.
This will helps to know whether we can use this device or not.
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 eng
Done
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
Done
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
Done
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?
byte, expected unit size is also byte
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 <tjeyasin at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nishanth Thomas <nishusemail at gmail.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtripat at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list