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