Change in vdsm[master]: Skip netdev probe from sessions missing iscsi sysfs (i.e. us...

nsoffer at redhat.com nsoffer at redhat.com
Thu Mar 5 16:42:37 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: Skip netdev probe from sessions missing iscsi sysfs (i.e. using hardware iSCSI)
......................................................................


Patch Set 2:

(5 comments)

https://gerrit.ovirt.org/#/c/38354/2/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:

Line 142
Line 143
Line 144
Line 145
Line 146
This is not related to your change, but using same temporaries for different types of data (path, contents of path) in the same function is evil.


Line 104:     Arguments:
Line 105:         sessionID - the iSCSI session ID.
Line 106:     Returns:
Line 107:         - iSCSI host path - e.g. '/sys/class/iscsi_host/host17/'
Line 108:         - None if host session is not present (i.e. hardware iSCSI)
How do we know that the missing path means "using hardware iscsi", and not "session was disconnected"?

Do we have some other sysfs attribute that can tell us that we should not expect this path?
Line 109:     """
Line 110: 
Line 111:     pattern = '/sys/devices/platform/host*/session%s' % sessionID
Line 112:     for path in glob.iglob(pattern):


Line 112:     for path in glob.iglob(pattern):
Line 113:         host = os.path.basename(os.path.dirname(path))
Line 114:         return '/sys/class/iscsi_host/' + host
Line 115: 
Line 116:     return None
It is more useful to raise here and handle the error in the caller.
Line 117: 
Line 118: 
Line 119: def readSessionInfo(sessionID):
Line 120:     iscsi_session = getIscsiSessionPath(sessionID)


Line 123:     iscsi_host = getIscsiHostPath(sessionID)
Line 124:     if iscsi_host is not None:
Line 125:         netdev = os.path.join(iscsi_host, "netdev")
Line 126:     else:
Line 127:         netdev = None
You can handle the OSError here:

    try:
        iscsi_host = getIscsiHostPath(sessionID)
        netdev = os.path.join(iscsi_host, "netdev")
    except OSError as e:
        if e.errno != errno.ENOENT:
            raise
        netdev = None

But the OSError we get is not a real error, we can make this code nicer by raising module private error:

    try:
        build netdev path...
    except NoSessionPath:
       netdev = None
Line 128: 
Line 129:     if not os.path.isdir(iscsi_session) or not os.path.isdir(iscsi_connection):
Line 130:         raise OSError(errno.ENOENT, "No such session")
Line 131: 


Line 143:     for fname in (targetname, iface, tpgt, user, passwd, paddr, pport, netdev):
Line 144:         try:
Line 145:             with open(fname, "r") as f:
Line 146:                 res.append(f.read().strip())
Line 147:         except (OSError, IOError, TypeError):
This code will silently hide None value for any of the names (targetname, iface, ...), while you are trying to handle None netdev.

Even if this could only fail when netdev is None, this is bad idea. You don't try to use None (open(fname, ...)) and depend on the fact that it raises TypeError; you should check explicitly for None and handle it.

The correct way would be to extract a function for reading path content, so we can write:

    for path in (values that must exits):
        append content at path

    if netdev is not None:
        append content at path

We probably need a helper to append contents of file at path.
Line 148:             res.append("")
Line 149: 
Line 150:     iqn, iface, tpgt, username, password, ip, port, netdev = res
Line 151:     port = int(port)


-- 
To view, visit https://gerrit.ovirt.org/38354
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6f273fddff2235f15197c6689de5e6374fda6f0
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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