Dan Kenigsberg has posted comments on this change. Change subject: Sysctl to allow iSCSI multipath with multiple NICs in the same subnet ......................................................................
Patch Set 15: Code-Review-1 (2 comments) http://gerrit.ovirt.org/#/c/31529/15/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1139: try: Line 1140: path = '/proc/sys/net/ipv4/conf/%s/rp_filter' % dev Line 1141: with open(path, 'w') as rp_filter: Line 1142: rp_filter.write(mode) Line 1143: except OSError as e: why do we need this error handling? Why do we want to ignore ENOENT? This function should fail hard if it fails to set the rp filter for the device dev. Line 1144: if e.errno == errno.ENOENT: Line 1145: logging.warning("Device %s does not exist", dev) Line 1146: else: Line 1147: logging.error("Error setting rp_filter mode [%s] for device [%s]", http://gerrit.ovirt.org/#/c/31529/15/vdsm/storage/iscsi.py File vdsm/storage/iscsi.py: Line 545: Line 546: return 0 Line 547: Line 548: Line 549: def _unsetRpFilter(sessionID): The name of this function is misleading since * it is named like setRpFilter(), but does not work like it * it does not always unset; it only tries to do so. too bad I don't have a good name. Maybe setStrictRpFilterIfUnique() Please a docstring explaining why this function is needed Line 550: sessionInfo = getSessionInfo(sessionID) Line 551: Line 552: if sessionInfo.iface.netIfacename: Line 553: for session in iterateIscsiSessions(): -- To view, visit http://gerrit.ovirt.org/31529 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibf93d49317c76aece764e53e58e0ff28868f16b0 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amador Pahim <apa...@redhat.com> Gerrit-Reviewer: Amador Pahim <apa...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
