Change in vdsm[master]: Configure iSCSI iface.net_ifacename
nsoffer at redhat.com
nsoffer at redhat.com
Sat Sep 13 08:53:45 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: Configure iSCSI iface.net_ifacename
......................................................................
Patch Set 11: Code-Review-1
(5 comments)
Partial review.
http://gerrit.ovirt.org/#/c/31534/11/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:
Line 106: - iSCSI host path - e.g. '/sys/class/host17/'
Line 107: - None, in case of iSCSI session's host not found
Line 108: """
Line 109: platform = os.path.join("/sys", "devices", "platform")
Line 110: iscsi_host = os.path.join("/sys", "class", "iscsi_host")
We don't need os.path.join to create linux specific constants.
Line 111:
Line 112: for subdir in os.listdir(platform):
Line 113: if subdir.startswith("host"):
Line 114: host_path = os.path.join(platform, subdir)
Line 113: if subdir.startswith("host"):
Line 114: host_path = os.path.join(platform, subdir)
Line 115: if os.path.exists(os.path.join(host_path, "session%s" %
Line 116: sessionId)):
Line 117: return os.path.join(iscsi_host, subdir)
Most of this code is re-implementation of the glob module.
Also returning None when value is not found is bad idea, as the other code in this patch prove - you forgot to check for None and your code blows. It is safer to raise an exception here.
Further, having session id without a host is impossible - right?
So this should be implemented like this (untested) code:
pattern = '/sys/class/iscsi_host/host*/session' + sessionId
for path in glob.glob(pattern):
return path
raise HostNotFound(pattern)
The exception is just a name I invented without looking at the code, we may already have a better error in this module, or in storage.storage_exception module.
Line 118:
Line 119:
Line 120: def readSessionInfo(sessionID):
Line 121: iscsi_session = getIscsiSessionPath(sessionID)
Line 119:
Line 120: def readSessionInfo(sessionID):
Line 121: iscsi_session = getIscsiSessionPath(sessionID)
Line 122: iscsi_connection = getIscsiConnectionPath(sessionID)
Line 123: iscsi_host = getIscsiHostPath(sessionID)
This can be None...
Line 124:
Line 125: if not os.path.isdir(iscsi_session) or not os.path.isdir(iscsi_connection):
Line 126: raise OSError(errno.ENOENT, "No such session")
Line 127:
Line 134:
Line 135: paddr = os.path.join(iscsi_connection, "persistent_address")
Line 136: pport = os.path.join(iscsi_connection, "persistent_port")
Line 137:
Line 138: netdev = os.path.join(iscsi_host, "netdev")
This will blow if iscsi_host is None
Line 139:
Line 140: res = []
Line 141: for fname in (targetname, iface, tpgt, user, passwd, paddr, pport, netdev):
Line 142: try:
Line 156: if password in ["<NULL>", "(null)"]:
Line 157: password = None
Line 158:
Line 159: if netdev in ["<NULL>", "(null)"]:
Line 160: netdev = None
Cannot happen
Line 161:
Line 162: iface = IscsiInterface(iface, netIfacename=netdev)
Line 163: portal = IscsiPortal(ip, port)
Line 164: target = IscsiTarget(portal, tpgt, iqn)
--
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: 11
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