Change in vdsm[master]: Configure iSCSI iface.net_ifacename
nsoffer at redhat.com
nsoffer at redhat.com
Mon Sep 15 21:56:19 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: Configure iSCSI iface.net_ifacename
......................................................................
Patch Set 12:
(2 comments)
Partial review
http://gerrit.ovirt.org/#/c/31534/12/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 107: Returns:
Line 108: - iSCSI host path - e.g. '/sys/class/iscsi_host/host17/'
Line 109: """
Line 110:
Line 111: pattern = os.path.join(_DEVPLATFORM_PATH, 'host*', 'session%s' % sessionID)
You still using os.path.join to create linux specific constant path - '/sys/devices/platform', 'host*', 'session%s'. I know that you can find similar bad examples like this in this module, but please do not add more of this.
Adding a constant in the top of the file only make it harder to understand and maintain, and give *no* added value.
Line 112: for path in glob.glob(pattern):
Line 113: session_host = os.path.basename(os.path.split(path)[0])
Line 114: return os.path.join(_ISCSIHOST_PATH, session_host)
Line 115:
Line 110:
Line 111: pattern = os.path.join(_DEVPLATFORM_PATH, 'host*', 'session%s' % sessionID)
Line 112: for path in glob.glob(pattern):
Line 113: session_host = os.path.basename(os.path.split(path)[0])
Line 114: return os.path.join(_ISCSIHOST_PATH, session_host)
This would be more clear as:
pattern = '/sys/devices/platform/host*/session%s' % sessionId
for path in glob.glob(pattern):
host = os.path.basename(os.path.dirname(path))
return '/sys/class/iscsi_session/' + host
Line 115:
Line 116: raise OSError(errno.ENOENT, "No iscsi_host for session [%s]" % sessionID)
Line 117:
Line 118:
--
To view, visit http://gerrit.ovirt.org/31534
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3ebb2f272669b753700b57486d869b21dd16f2d6
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv 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