Nir Soffer has posted comments on this change. Change subject: ipwrapper: switch link polling to netlink ......................................................................
Patch Set 9: (15 comments) See 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 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 be nice if you explain what are you doing here, and link to the secret place in ctypes docs for more info. 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 when the module starts with the public interface, and the private stuff is at the bottom. Decide if you are using camelCase or underscore_style. Since you call this link_info, "data" should be "info". I think it can be little more clear if you start with empty info and add stuff, instead of creating info as a one item literal dict. info = {} info['index'] = _rtnl_link_get_ifindex(link) ... return info 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. Or, can you extract a function that return the address? def _getLinkAddress(link): nl_addr = _rtnl_link_get_addr(link) if nl_addr: address = (c_char * CHARBUFFSIZE)() _nl_addr2str(nl_addr, address, sizeof(address)) return address.value return None And now the this code is simply: data["address"] = _getLinkAddress(link) 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. 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? 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 77: #data['type'] = ctypes.cast(LIBNL.rtnl_link_get_info_type(link), Line 78: #ctypes.c_char_p).value 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) And here, or extract _getLinkDevice function. Line 82: underlying_device = _rtnl_link_get_link(link) Line 83: if underlying_device > 0: Line 84: device_name = (c_char * CHARBUFFSIZE)() Line 85: data['device'] = _rtnl_link_i2name(cache, underlying_device, Line 87: Line 88: vlanid = _rtnl_link_vlan_get_id(link) Line 89: if vlanid >= 0: Line 90: data['vlanid'] = vlanid Line 91: Extract _getLinkMaster function? Line 92: master = _rtnl_link_get_master(link) Line 93: if master >= 1: Line 94: master_name = (c_char * CHARBUFFSIZE)() Line 95: data['master'] = _rtnl_link_i2name(cache, master, master_name, Line 93: if master >= 1: Line 94: master_name = (c_char * CHARBUFFSIZE)() Line 95: data['master'] = _rtnl_link_i2name(cache, master, master_name, Line 96: sizeof(master_name)) Line 97: Extract _getLinkState function? Line 98: state = _rtnl_link_get_operstate(link) Line 99: operstate = (c_char * CHARBUFFSIZE)() Line 100: _rtnl_link_operstate2str(state, operstate, sizeof(operstate)) Line 101: data['state'] = operstate.value 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 that it is actually an iterator. How about renaming it to "iterLinks"? for link in netlink.iterLinks(): ... link = netlink.getLink('bob') 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. 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. 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? 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 all requests? I guess this will be less error prone, and less code. 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. 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
