Change in vdsm[master]: netlink: User libnl-3 when available

nsoffer at redhat.com nsoffer at redhat.com
Tue Apr 8 12:53:46 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: netlink: User libnl-3 when available
......................................................................


Patch Set 2:

(7 comments)

http://gerrit.ovirt.org/#/c/26514/2//COMMIT_MSG
Commit Message:

Line 10: libnl3 while vdsm was using libnl1. The problem was that libnl1
Line 11: and libnl3 use the same algorithm for allocating netlink ports and
Line 12: collided. This patch fixes it by using libnl3 if it is in the system
Line 13: (which reasonably means that the python-ethtool version will be new
Line 14: enough as to use it as well)
Is it possible that ethtool is old and the system does have libnl3?
Line 15: 
Line 16: Change-Id: I45f043a8416e57d5fb550e2e817e6df38cd793b1
Line 17: Bug-Url: https://bugzilla.redhat.com/1078312


http://gerrit.ovirt.org/#/c/26514/2/lib/vdsm/netlink.py
File lib/vdsm/netlink.py:

Line 43: 
Line 44:     _nl_socket_alloc = CFUNCTYPE(c_void_p)(('nl_socket_alloc', LIBNL3))
Line 45:     _nl_socket_free = _none_proto(('nl_socket_free', LIBNL3))
Line 46: 
Line 47:     LIBNL_ROUTE = CDLL('libnl-route-3.so',  use_errno=True)
Lets initialize both LIBNL and LIBNL_ROUTE together, and separate the version check to separate variable:

try:
    LIBNL = CDLL('libnl-3.so.200', use_errno=True)
    LIBNL_ROUTE = CDLL('libnl-route-3.so', use_errno=True)
    has_libnl3 = True
    ....
except ...
    LIBNL_ROUTE = LIBNL = CDLL('libnl.so.1', use_errno=True)
    has_libnl3 = False

Now we can use the has_libnl3 later, making it easier to follow. And it is very clear now why we have both LIBNL and LIBNL_ROUTE.
Line 48: 
Line 49:     def _rtnl_link_alloc_cache(sock):
Line 50:         """Wraps the new link alloc cache to expose the libnl1 signature"""
Line 51:         AF_UNSPEC = 0


Line 48: 
Line 49:     def _rtnl_link_alloc_cache(sock):
Line 50:         """Wraps the new link alloc cache to expose the libnl1 signature"""
Line 51:         AF_UNSPEC = 0
Line 52:         __rtnl_link_alloc_cache = CFUNCTYPE(c_int, c_void_p, c_int, c_void_p)(
Do we want to run this CFUNCTYPE on every call?

Another private temporary with too long name
Line 53:             ('rtnl_link_alloc_cache', LIBNL_ROUTE))
Line 54:         cache = c_void_p()
Line 55:         err = __rtnl_link_alloc_cache(sock, AF_UNSPEC, byref(cache))
Line 56:         if err:


Line 55:         err = __rtnl_link_alloc_cache(sock, AF_UNSPEC, byref(cache))
Line 56:         if err:
Line 57:             return None
Line 58:         else:
Line 59:             return cache
This wil be cleaner as:

    err = __rtnl_link_alloc_cache(sock, AF_UNSPEC, byref(cache))
    if err:
        return None

    return cache

But do we really want to return None? why not raise and handle it upper?
Line 60: 
Line 61:     def _rtnl_addr_alloc_cache(sock):
Line 62:         """Wraps the new addr alloc cache to expose the libnl1 signature"""
Line 63:         __rtnl_addr_alloc_cache = CFUNCTYPE(c_int, c_void_p, c_void_p)(


Line 74:     def _rtnl_link_vlan_get_id(link):
Line 75:         """Wraps the new vlan id retrieval to expose the libnl1 signature"""
Line 76:         __rtnl_link_is_vlan = _int_proto(('rtnl_link_is_vlan', LIBNL_ROUTE))
Line 77:         __rtnl_link_vlan_get_id = _int_proto(('rtnl_link_vlan_get_id',
Line 78:                                              LIBNL_ROUTE))
Do we really want to invoke these lines on each call to this function?

And even if we do, why do we need __private names for temporary variables? and why the names are so long?

For example, in the scope of this function, there is only one type of link.  Name like is_vlan is just what we need:

    if is_vlan(link):
        return get_id(link)
    else:
        return -1
Line 79:         if __rtnl_link_is_vlan(link):
Line 80:             return __rtnl_link_vlan_get_id(link)
Line 81:         else:
Line 82:             return -1


Line 78:                                              LIBNL_ROUTE))
Line 79:         if __rtnl_link_is_vlan(link):
Line 80:             return __rtnl_link_vlan_get_id(link)
Line 81:         else:
Line 82:             return -1
This ifdef like block is little ugly, but I don't see how to do it better. Can we move this to the end of the file, were the details belong?

It is easier to work with code that put the public api first, then the private apis, and finally the nasty parts, were you usually do not see them.
Line 83: 
Line 84: except OSError:  # libnl3 is not present, fallback to libnl1
Line 85:     # (ctypes does not set an errno for this case)
Line 86:     LIBNL_ROUTE = LIBNL = CDLL('libnl.so.1', use_errno=True)


Line 159: 
Line 160: 
Line 161: def _open_socket():
Line 162:     """Returns an open netlink socket."""
Line 163:     sock = _nl_socket_alloc()
Are these naming changes related to this patch?

Please separate them if not.
Line 164:     if sock is None:
Line 165:         raise IOError(get_errno(), 'Failed to allocate netlink handle')
Line 166: 
Line 167:     err = _nl_connect(sock, NETLINK_ROUTE)


-- 
To view, visit http://gerrit.ovirt.org/26514
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I45f043a8416e57d5fb550e2e817e6df38cd793b1
Gerrit-PatchSet: 2
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: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list