Antoni Segura Puimedon has posted comments on this change. Change subject: ipwrapper: switch link polling to netlink ......................................................................
Patch Set 1: Verified-1 (7 comments) For some reason the code works perfectly when running from the process main thread but fails from other threads. Investigating... http://gerrit.ovirt.org/#/c/23248/1/lib/vdsm/ipwrapper.py File lib/vdsm/ipwrapper.py: Line 138: Line 139: @classmethod Line 140: def fromDict(cls, data): Line 141: # TODO: Tune the following line to reuse the type when libnl1 type Line 142: # getting is fixed. > do you have a BZ open for this issue, so we can tell when it's safe to remo Only upstream bugzilla, not on RH. Line 143: data['linkType'] = _detectType(data['name']) Line 144: return cls(**data) Line 145: Line 146: @classmethod http://gerrit.ovirt.org/#/c/23248/1/lib/vdsm/netlink.py File lib/vdsm/netlink.py: Line 11: data = {'index': LIBNL.rtnl_link_get_ifindex(link)} Line 12: data['name'] = ctypes.cast(LIBNL.rtnl_link_get_name(link), Line 13: ctypes.c_char_p).value Line 14: nl_addr = LIBNL.rtnl_link_get_addr(link) Line 15: address = (ctypes.c_char * 32)() > address should be allocated only when nl_addr is non-null. Done Line 16: if nl_addr: Line 17: LIBNL.nl_addr2str(nl_addr, address, Line 18: ctypes.sizeof(address)) Line 19: data['address'] = address.value Line 33: data['vlanid'] = vlanid Line 34: Line 35: master = LIBNL.rtnl_link_get_master(link) Line 36: if master >= 1: Line 37: master_name = (ctypes.c_char * 32)() > Why 32 and not IFNAMSIZ? May we define CONSTANTS for magic numbers? Done Line 38: data['master'] = ctypes.cast( Line 39: LIBNL.rtnl_link_i2name(cache, master, master_name, Line 40: ctypes.sizeof(master_name)), Line 41: ctypes.c_char_p).value Line 51: with _nl_link_cache() as cache: Line 52: link = LIBNL.nl_cache_get_first(cache) Line 53: links = [] Line 54: while link: Line 55: links.append(_link_info(cache, link)) > Wouldn't it be nicer to yield the links as they are found, over creating a I'll give it a shot. Line 56: link = LIBNL.nl_cache_get_next(link) Line 57: return links Line 58: Line 59: Line 76: if err: Line 77: raise IOError(err, 'Failed to connect to netlink socket.') Line 78: yield handle Line 79: finally: Line 80: # handle is closed automatically on destroy. > what is "automatic" in this explicit destroy? That I don't call disconnect/close, nl_handle_destroy does it for me. Line 81: LIBNL.nl_handle_destroy(handle) Line 82: Line 83: Line 84: @contextmanager Line 86: """Provides a link cache and frees it and its links upon exit.""" Line 87: with _open_nl_socket() as sock: Line 88: cache = LIBNL.rtnl_link_alloc_cache(sock) Line 89: if cache is None: Line 90: raise IOError('Failed to allocate link cache.') > better invent an errno if none is available, over raising a badly-types IOE Done Line 91: try: Line 92: yield cache Line 93: finally: http://gerrit.ovirt.org/#/c/23248/1/tests/ipwrapperTests.py File tests/ipwrapperTests.py: Line 299: def testGetLink(self): Line 300: link = ipwrapper.getLink(self._bridge.devName) Line 301: self.assertTrue(link.isBRIDGE) Line 302: self.assertEqual(link.master, None) Line 303: self.assertEqual(link.mtu, 1500) > I do not expect Linux to ever change it's default, but we'd better set it e Done Line 304: self.assertEqual(link.name, self._bridge.devName) Line 305: Line 306: Line 307: class TestDrvinfo(TestCaseBase): -- To view, visit http://gerrit.ovirt.org/23248 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09a120155e3c5be15c237171620e5c996c2af681 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Assaf Muller <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
