Federico Simoncelli has posted comments on this change. Change subject: Handle iscsi iface.net_ifacename for existing iface ......................................................................
Patch Set 3: Code-Review+1 (1 comment) Dan upgrade to +2 if this is good enough for you. http://gerrit.ovirt.org/#/c/35976/3/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 153: Line 154: for iface in iscsi.iterateIscsiInterfaces(): Line 155: if iface.name == ifaceName: Line 156: if netIfaceName is not None: Line 157: if iface.netIfaceName is None: Some comments: * it was hard to find the positive case that we're interested about, what we're really looking for here (just as for the name) is the case where netIfaceName == iface.netIfaceName (not sure if there's anything we can do about this one, I didn't find any good suggestion to give here) * I would have moved the "update" logic to its own function My personal taste would have gone for something like: for iface in iscsi.iterateIscsiInterfaces(): if iface.name != ifaceName: continue if netIfaceName is not None: updateIfaceNameIfNeeded(...) if iface.netIfaceName != netIfaceName: logging.error(...) raise se.iSCSIifaceError() return iface Line 158: iface.netIfaceName = netIfaceName Line 159: iface.update() Line 160: elif netIfaceName != iface.netIfaceName: Line 161: logging.error('iSCSI netIfaceName coming from engine [%s] ' -- To view, visit http://gerrit.ovirt.org/35976 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I18c2b92a75ee3bcbbcc546e2bfa4f133a5f9ad22 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amador Pahim <[email protected]> Gerrit-Reviewer: Amador Pahim <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
