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