Change in vdsm[master]: netlink: Introduce socket pool

nsoffer at redhat.com nsoffer at redhat.com
Thu Feb 20 21:54:30 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: netlink: Introduce socket pool
......................................................................


Patch Set 5: Code-Review+1

(8 comments)

Very nice code, but can be cleaned up a bit easily.

http://gerrit.ovirt.org/#/c/24603/5/lib/vdsm/netlink.py
File lib/vdsm/netlink.py:

Line 82:     'rtnl_scope2str', LIBNL))
Line 83: 
Line 84: 
Line 85: class NLSocketPoolError(Exception):
Line 86:     pass
Looks like unused class - kill it?
Line 87: 
Line 88: 
Line 89: class NLSocketPool(object):
Line 90:     """Pool of netlink sockets."""


Line 88: 
Line 89: class NLSocketPool(object):
Line 90:     """Pool of netlink sockets."""
Line 91:     def __init__(self, size):
Line 92:         assert size > 0
It will little nicer to raise here ValueError:

    if size <= 0:
        raise ValueError("Invalid pool size: %r (size <= 0)", size)
Line 93:         self._size = size
Line 94:         self._sockets = set()
Line 95:         self._semaphore = BoundedSemaphore(size)
Line 96: 


Line 89: class NLSocketPool(object):
Line 90:     """Pool of netlink sockets."""
Line 91:     def __init__(self, size):
Line 92:         assert size > 0
Line 93:         self._size = size
We can get rid of _size
Line 94:         self._sockets = set()
Line 95:         self._semaphore = BoundedSemaphore(size)
Line 96: 
Line 97:     @contextmanager


Line 91:     def __init__(self, size):
Line 92:         assert size > 0
Line 93:         self._size = size
Line 94:         self._sockets = set()
Line 95:         self._semaphore = BoundedSemaphore(size)
And lets initialize the _semphore before the _sockets, since it is more important. Just my personal style if you like to adopt it.
Line 96: 
Line 97:     @contextmanager
Line 98:     def socket(self):
Line 99:         """Takes a netlink socket from the pool and returns it afterwards."""


Line 95:         self._semaphore = BoundedSemaphore(size)
Line 96: 
Line 97:     @contextmanager
Line 98:     def socket(self):
Line 99:         """Takes a netlink socket from the pool and returns it afterwards."""
More correct docstring would describe that this return a socket from the pool, and increase the pool if needed.
Line 100:         with self._semaphore:
Line 101:             try:
Line 102:                 sock = self._sockets.pop()
Line 103:             except KeyError:


Line 100:         with self._semaphore:
Line 101:             try:
Line 102:                 sock = self._sockets.pop()
Line 103:             except KeyError:
Line 104:                 sock = _open_socket()
This works, but it would be more clear to write:

    if self._sockets:
        sock = self._sockets.pop()
    else:
        sock = _open_socket()
Line 105:             try:
Line 106:                 yield sock
Line 107:             finally:
Line 108:                 self._sockets.add(sock)


Line 118:     if _pool is None:
Line 119:         with _pool_lock:
Line 120:             if _pool is None:
Line 121:                 _pool = NLSocketPool(_POOL_SIZE)
Line 122:     return _pool
Now that sockets are created lazily, we create create the pool during import, and we don't need _get_pool and _pool_lock, and Lock:

    _pool = NLSocketPool(_POOL_SIZE)

and later in iter_links():

    with _pool.socket() as sock:
        ...
Line 123: 
Line 124: 
Line 125: def _open_socket():
Line 126:     """Returns an open netlink socket."""


Line 129:         raise IOError(get_errno(), 'Failed to allocate netlink handle')
Line 130: 
Line 131:     err = _nl_connect(handle, NETLINK_ROUTE)
Line 132:     if err:
Line 133:         _close_socket(handle)
Here using _nl_handle_destroy is more correct, since here you are in libnl territory.
Line 134:         raise IOError(-err, _nl_geterror())
Line 135:     return handle
Line 136: 
Line 137: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I657ac3d3e0c2661ce73bdef9aa807ead888a42eb
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list