Dan Kenigsberg has posted comments on this change. Change subject: netlink: event monitor (even more simple version) ......................................................................
Patch Set 17: (5 comments) http://gerrit.ovirt.org/#/c/36197/17/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 81: self._queue = Queue.Queue() Line 82: self._running = False Line 83: Line 84: def __iter__(self): Line 85: getter = self._queue.get if self._running else self._queue.get_nowait if we are using the _STOP_QUEUE sentinel, why do we ever need get_nowait()? If the thread is not yet running - we should block. If the thread is not longer running - we would have _STOP_QUEUE in the queue. Please make sure both of these cases are tested. Line 86: try: Line 87: for event in iter(getter, None): Line 88: if event == _STOP_QUEUE: Line 89: break Line 97: scan_thread.start() Line 98: self._running = True Line 99: Line 100: def _scan(self): Line 101: epoll = select.epoll() for the coolness factor (and 1 line less): with closing(select.epoll()) as epoll: Line 102: try: Line 103: with _monitoring_socket(self._queue, self._groups, epoll) as sock: Line 104: with _pipetrick(epoll) as self._pipetrick: Line 105: while True: Line 105: while True: Line 106: events = NoIntrPoll(epoll.poll) Line 107: if (self._pipetrick[0], select.POLLIN) in events: Line 108: self._queue.put(_STOP_QUEUE) Line 109: os.read(self._pipetrick[0], 1) Please add a TODO regarding NoIntrCall Line 110: break Line 111: _nl_recvmsgs_default(sock) Line 112: finally: Line 113: epoll.close() Line 193: argument (monitor's queue in this case) Line 194: """ Line 195: nl_error = _nl_msg_parse(msg, _c_object_input, queue) Line 196: if nl_error < 0: Line 197: logging.error('EventMonitor nl_msg_parse() failed with %d\n' % here, there's no need in a trailing \n. Also, please use logging.error('EventMonitor nl_msg_parse() failed with %d\n', nl_error) to avoid formatting if loglevel is low. Line 198: nl_error) Line 199: return _NL_STOP Line 200: _c_event_input = CFUNCTYPE(c_int, c_void_p, c_void_p)(_event_input) Line 201: Line 228: yield pipetrick Line 229: finally: Line 230: epoll.unregister(pipetrick[0]) Line 231: finally: Line 232: os.close(pipetrick[0]) os.pipe() returns TWO new file descriptors. Here you close only the first. You should close the second, too. Line 233: Line 234: -- To view, visit http://gerrit.ovirt.org/36197 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0f4fcfde87ad51eb832f54862371b4da1281826e Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
