Antoni Segura Puimedon has posted comments on this change. Change subject: ipwrapper: switch link polling to netlink ......................................................................
Patch Set 9: (12 comments) http://gerrit.ovirt.org/#/c/23248/9/lib/vdsm/netlink.py File lib/vdsm/netlink.py: Line 25: NETLINK_ROUTE = 0 Line 26: CHARBUFFSIZE = 32 Line 27: LIBNL = CDLL('libnl.so.1', use_errno=True) Line 28: Line 29: # C function specs > Looks like C function prototypes Done Line 30: _int_proto = CFUNCTYPE(c_int, c_void_p) Line 31: _char_proto = CFUNCTYPE(c_char_p, c_void_p) Line 32: _void_proto = CFUNCTYPE(c_void_p, c_void_p) Line 33: Line 29: # C function specs Line 30: _int_proto = CFUNCTYPE(c_int, c_void_p) Line 31: _char_proto = CFUNCTYPE(c_char_p, c_void_p) Line 32: _void_proto = CFUNCTYPE(c_void_p, c_void_p) Line 33: > Since this is badly documented and mostly unknown ctypes voodoo, it would b Done Line 34: _nl_connect = CFUNCTYPE(c_int, c_void_p, c_int)(('nl_connect', LIBNL)) Line 35: _nl_handle_alloc = CFUNCTYPE(c_void_p)(('nl_handle_alloc', LIBNL)) Line 36: _nl_handle_destroy = CFUNCTYPE(None, c_void_p)(('nl_handle_destroy', LIBNL)) Line 37: Line 60: _rtnl_link_operstate2str = CFUNCTYPE(c_char_p, c_int, c_char_p, c_size_t)(( Line 61: 'rtnl_link_operstate2str', LIBNL)) Line 62: Line 63: Line 64: def _link_info(cache, link): > Can you move this to the bottom of the file? I think it help to understand Done Line 65: data = {'index': _rtnl_link_get_ifindex(link)} Line 66: data['name'] = _rtnl_link_get_name(link) Line 67: nl_addr = _rtnl_link_get_addr(link) Line 68: if nl_addr: Line 62: Line 63: Line 64: def _link_info(cache, link): Line 65: data = {'index': _rtnl_link_get_ifindex(link)} Line 66: data['name'] = _rtnl_link_get_name(link) > Little more whitespace between different groups may help. Done Line 67: nl_addr = _rtnl_link_get_addr(link) Line 68: if nl_addr: Line 69: address = (c_char * CHARBUFFSIZE)() Line 70: _nl_addr2str(nl_addr, address, sizeof(address)) Line 69: address = (c_char * CHARBUFFSIZE)() Line 70: _nl_addr2str(nl_addr, address, sizeof(address)) Line 71: data['address'] = address.value Line 72: else: Line 73: data['address'] = None > More whitespace here before the note. Done Line 74: # TODO: libnl-1 has a bug Line 75: # https://github.com/tgraf/libnl-1.1-stable/issues/1 Line 76: # when getting type. Add a call to the following line when fixed. Line 77: #data['type'] = ctypes.cast(LIBNL.rtnl_link_get_info_type(link), Line 74: # TODO: libnl-1 has a bug Line 75: # https://github.com/tgraf/libnl-1.1-stable/issues/1 Line 76: # when getting type. Add a call to the following line when fixed. Line 77: #data['type'] = ctypes.cast(LIBNL.rtnl_link_get_info_type(link), Line 78: #ctypes.c_char_p).value > And after it? Done Line 79: data['flags'] = _rtnl_link_get_flags(link) Line 80: data['qdisc'] = _rtnl_link_get_qdisc(link) Line 81: data['mtu'] = _rtnl_link_get_mtu(link) Line 82: underlying_device = _rtnl_link_get_link(link) Line 101: data['state'] = operstate.value Line 102: return data Line 103: Line 104: Line 105: def getLinks(): > getLinks and getLink are little too similar. Also getLinks does not explain Done Line 106: with _nl_link_cache() as cache: Line 107: link = _nl_cache_get_first(cache) Line 108: while link: Line 109: yield _link_info(cache, link) Line 106: with _nl_link_cache() as cache: Line 107: link = _nl_cache_get_first(cache) Line 108: while link: Line 109: yield _link_info(cache, link) Line 110: link = _nl_cache_get_next(link) > This is poetry. Thanks ;-) Line 111: Line 112: Line 113: def getLink(name): Line 114: with _nl_link_cache() as cache: Line 119: return _link_info(cache, link) Line 120: Line 121: Line 122: @contextmanager Line 123: def _open_nl_socket(): > Decide if you are using camelCase or underscore_style. Done Line 124: handle = _nl_handle_alloc() Line 125: if handle is None: Line 126: raise IOError(get_errno(), 'Failed to allocate netlink handle') Line 127: try: Line 124: handle = _nl_handle_alloc() Line 125: if handle is None: Line 126: raise IOError(get_errno(), 'Failed to allocate netlink handle') Line 127: try: Line 128: err = _nl_connect(handle, NETLINK_ROUTE) > Does it return errno value? Good catch, it is -errno. Changed! Line 129: if err: Line 130: raise IOError(err, 'Failed to connect to netlink socket.') Line 131: yield handle Line 132: finally: Line 131: yield handle Line 132: finally: Line 133: # handle is automatically disconnected on destroy. Line 134: _nl_handle_destroy(handle) Line 135: > Can we have one socket, opened and connected on startup, and reuse it for a It wouldn't be safe, as we get requests from multiple threads and it would be troublesome that they shared the socket. python-ethtool is also conservative in this and we are to move to that later, I'd rather have the same concurrency principle. Line 136: Line 137: @contextmanager Line 138: def _nl_link_cache(): Line 139: """Provides a link cache and frees it and its links upon exit.""" Line 134: _nl_handle_destroy(handle) Line 135: Line 136: Line 137: @contextmanager Line 138: def _nl_link_cache(): > Decide if you are using camelCase or underscore_style. Done Line 139: """Provides a link cache and frees it and its links upon exit.""" Line 140: with _open_nl_socket() as sock: Line 141: cache = _rtnl_link_alloc_cache(sock) Line 142: if cache is None: -- 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: 9 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: Nir Soffer <[email protected]> Gerrit-Reviewer: Ondřej Svoboda <[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
