Change in vdsm[master]: iscsiadm.iface_list() to return iface.net_ifacename

danken at redhat.com danken at redhat.com
Tue Dec 2 21:35:47 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: iscsiadm.iface_list() to return iface.net_ifacename
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

http://gerrit.ovirt.org/#/c/35807/1/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:

Line 404: def iterateIscsiInterfaces():
Line 405:     ifaces = iscsiadm.iface_list()
Line 406:     for iface in ifaces:
Line 407:         name = iface[0]
Line 408:         if iface[4] == '<empty>':
I think that this '<empty>' string is an artifact of the iscsiadm utility, so this translation to None should happen in iscsiadm.py.
Line 409:             netIfaceName = None
Line 410:         else:
Line 411:             netIfaceName = iface[4]
Line 412: 


http://gerrit.ovirt.org/#/c/35807/1/vdsm/storage/iscsiadm.py
File vdsm/storage/iscsiadm.py:

Line 95: def iface_exists(interfaceName):
Line 96:     # FIXME: can be optimized by checking /var/lib/iscsi/ifaces
Line 97:     for iface in iface_list():
Line 98:         if interfaceName == iface[0]:
Line 99:             return interfaceName
?

this function used to return True/False. Now it returns str/None. I don't suppose it's intentional.
Line 100: 
Line 101: 
Line 102: def iface_new(name):
Line 103:     if name in _RESERVED_INTERFACES:


Line 141:     # /var/lib/iscsi/ifaces. Fix if ever a performance bottleneck.
Line 142:     # "iscsiadm -m iface" output format:
Line 143:     #   <iscsi_ifacename> <transport_name>,<hwaddress>,<ipaddress>,\
Line 144:     #   <net_ifacename>,<initiatorname>
Line 145:     ifaces = ()
please return either a list of tuples or a tuple of tuples. don't mix.
Line 146:     if out is None:
Line 147:         rc, out, err = _runCmd(["-m", "iface"])
Line 148:         if rc != 0:
Line 149:             raise IscsiInterfaceListingError(rc, out, err)


Line 148:         if rc != 0:
Line 149:             raise IscsiInterfaceListingError(rc, out, err)
Line 150: 
Line 151:     for line in out:
Line 152:         iface = [line.split()[0]]
calling line.split() twice is wasteful and less pretty.

please convert each line to a tuple, not a list, since each iface info is immutable. a named tuple could be even cooler.
Line 153:         iface.extend(line.split()[1].split(','))
Line 154: 
Line 155:         ifaces = ifaces + (iface,)
Line 156: 


-- 
To view, visit http://gerrit.ovirt.org/35807
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6d0192860b53e24524f58245c92d45f8146bc725
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce 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