Change in vdsm[master]: ipwrapper: switch link polling to netlink
asegurap at redhat.com
asegurap at redhat.com
Mon Jan 20 15:42:11 UTC 2014
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 <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list