Dan Kenigsberg has posted comments on this change. Change subject: ipwrapper: switch link polling to netlink ......................................................................
Patch Set 1: Code-Review-1 (9 comments) Few comments and questions. 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 remove? 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 1: from contextlib import contextmanager boiler plate missing. We should automate this check.. Line 2: import ctypes Line 3: import errno Line 4: Line 5: 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. 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 18: ctypes.sizeof(address)) Line 19: data['address'] = address.value Line 20: else: Line 21: data['address'] = None Line 22: # TODO: libnl-1 has a bug when getting type. Add the following line when please add the reference to the bug. Line 23: # fixed. Line 24: #data['type'] = ctypes.cast(LIBNL.rtnl_link_get_info_type(link), Line 25: #ctypes.c_char_p).value Line 26: data['flags'] = LIBNL.rtnl_link_get_flags(link) 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? 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 list()? 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? 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 IOError. 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 explicitly if we want to verify MTU. 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 <asegu...@redhat.com> Gerrit-Reviewer: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Ondřej Svoboda <osvob...@redhat.com> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
