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