Change in vdsm[master]: netlink: event monitor

danken at redhat.com danken at redhat.com
Fri Dec 12 12:47:17 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: netlink: event monitor
......................................................................


Patch Set 38: Code-Review-1

(7 comments)

http://gerrit.ovirt.org/#/c/32626/38/lib/vdsm/netlink/__init__.py
File lib/vdsm/netlink/__init__.py:

Line 31: 
Line 32: _NLE_NODEV = 31  # libnl/incluede/netlink/errno.h
Line 33: 
Line 34: _NL_CB_DEFAULT = 0  # libnl/include/netlink/handlers.h
Line 35: _NL_CB_CUSTOM = 3   # libnl/include/netlink/handlers.h
newline begs to be written here.
Line 36: _NL_STOP = 2        # libnl/python/netlink/capi.i
Line 37: 
Line 38: _NL_ROUTE_NAME = 'route'
Line 39: _NL_ROUTE_ADDR_NAME = _NL_ROUTE_NAME + '/addr'  # libnl/lib/route/addr.c


Line 173: _int_char_proto = CFUNCTYPE(c_char_p, c_int, c_char_p, c_size_t)
Line 174: _char_proto = CFUNCTYPE(c_char_p, c_void_p)
Line 175: _void_proto = CFUNCTYPE(c_void_p, c_void_p)
Line 176: _none_proto = CFUNCTYPE(None, c_void_p)
Line 177: # 14 supported groups (int), last parameter has to be 0
Would 

 CFUNCTYPE(*(c_int, c_void_p) + (c_int, ) * (len(_KNOWN_GROUPS)+1))

fly?
Line 178: _socket_memberships_proto = CFUNCTYPE(c_int, c_void_p, c_int, c_int, c_int,
Line 179:                                       c_int, c_int, c_int, c_int, c_int, c_int,
Line 180:                                       c_int, c_int, c_int, c_int, c_int, c_int)
Line 181: 


http://gerrit.ovirt.org/#/c/32626/38/lib/vdsm/netlink/monitor.py
File lib/vdsm/netlink/monitor.py:

Line 80:         self._raise_timeout_error = raise_timeout_error
Line 81:         self._thread = MonitorThread(self._queue, self._groups, self._timeout)
Line 82: 
Line 83:     def __iter__(self):
Line 84:         try:
the scope of this try-block should be only the get_nowait() call, iianm.
Line 85:             while True:
Line 86:                 if self._thread.is_alive():
Line 87:                     value = self._thread._queue.get()
Line 88:                     if value is _TIMEOUT_CODE:


Line 114:         self._thread.join()
Line 115: 
Line 116: 
Line 117: # libnl/include/linux/rtnetlink.h
Line 118: _EVENTS = {16: 'new_link',            # RTM_NEWLINK
I personally prefer the simple 4-spaces indentation. Particularly on big blocks like this.
Line 119:            17: 'del_link',            # RTM_DELLINK
Line 120:            18: 'get_link',            # RTM_GETLINK
Line 121:            19: 'set_link',            # RTM_SETLINK
Line 122:            20: 'new_addr',            # RTM_NEWADDR


Line 212: 
Line 213:         try:
Line 214:             _add_socket_memberships(self.sock, self._groups)
Line 215:             while not self._stop.is_set():
Line 216:                 if self._timeout:
this is a bit too complex for me...

can you split the timeout feature to another patch? At the moment, whoever want to timeout the monitor, calls mon.stop() in Timeout. Avoiding an extra thread is a nice feature, but it can wait for a followup patch.
Line 217:                     if (time.time() - self._timeStart > self._timeout):
Line 218:                         self._stop.set()
Line 219:                         self._queue.put_nowait(_TIMEOUT_CODE)
Line 220:                         continue


Line 218:                         self._stop.set()
Line 219:                         self._queue.put_nowait(_TIMEOUT_CODE)
Line 220:                         continue
Line 221:                 fd = _nl_socket_get_fd(self.sock)
Line 222:                 if select.select([fd], [], [], 0.1) != ([], [], []):
use poll, which scales much better.

Also, instead of waking up 10 times each second, I think Saggi had a trick for waiting on an fd -or- an event.
Line 223:                     _nl_recvmsgs_default(self.sock)
Line 224:         finally:
Line 225:             _drop_socket_memberships(self.sock, self._groups)
Line 226:             _close_socket(self.sock)


Line 221:                 fd = _nl_socket_get_fd(self.sock)
Line 222:                 if select.select([fd], [], [], 0.1) != ([], [], []):
Line 223:                     _nl_recvmsgs_default(self.sock)
Line 224:         finally:
Line 225:             _drop_socket_memberships(self.sock, self._groups)
this should happen only if _add_socket_memberships()
Line 226:             _close_socket(self.sock)
Line 227: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I23ea72986564c5a115e36be0e7cf679c28c8ea96
Gerrit-PatchSet: 38
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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