Change in vdsm[master]: iscsi: Use tpgt when login, update or remove iscsi node

sgotliv at redhat.com sgotliv at redhat.com
Sun Mar 16 11:29:49 UTC 2014


Sergey Gotliv has posted comments on this change.

Change subject: iscsi: Use tpgt when login, update or remove iscsi node
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.ovirt.org/#/c/25706/3/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 99: QEMU_READABLE_TIMEOUT = 30
Line 100: 
Line 101: HSM_DOM_MON_LOCK = "HsmDomainMonitorLock"
Line 102: 
Line 103: DEFAULT_TPGT = 1
> This does not look related to hsm. Why not define this in iscsi module?
Ok, I'll move it to iscsi.py
Line 104: 
Line 105: def public(f=None, **kwargs):
Line 106:     if f is None:
Line 107:         return partial(public, **kwargs)


Line 226:     elif typeName == 'iscsi':
Line 227:         portal = iscsi.IscsiPortal(
Line 228:             conDict.get('connection', None),
Line 229:             int(conDict.get('port', None)))
Line 230:         tpgt = conDict.get('tpgt', DEFAULT_TPGT)
> Why not use None like other values, and let iscsi.IscsiTarget() define a de
IscsiTarget is the namedtuple. Of course I can change it to be a class but its kind of not related change especially if I can assign the right default value in the first place.

Just to be clear I don't think that assigning a None as a default and then using "1" as default doesn't make it more clear and understandable.
Line 231: 
Line 232:         target = iscsi.IscsiTarget(portal, tpgt, conDict.get('iqn', None))
Line 233: 
Line 234:         iface = _resolveIscsiIface(conDict.get('ifaceName', None),


http://gerrit.ovirt.org/#/c/25706/3/vdsm/storage/iscsi.py
File vdsm/storage/iscsi.py:

Line 143: 
Line 144: 
Line 145: def addIscsiNode(iface, target, credentials=None):
Line 146:     targetName = target.iqn
Line 147:     portalStr = "%s:%d,%d" % (target.portal.hostname, target.portal.port,
> Since other place use this name to portal without a tpgt, you should use he
Iscsi uses the term portal. PortalStr is a bad name (I would make it just a "portal"), but fullPortalStr is even worse because it doesn't describe any existing iscsi term.
Line 148:                               target.tpgt)
Line 149:     with _iscsiadmTransactionLock:
Line 150:         iscsiadm.node_new(iface.name, portalStr, targetName)
Line 151:         try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I11a6a556d31524f4fae4786f5735accaf1cefb34
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce 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