Dan Kenigsberg has posted comments on this change. Change subject: netlink: event monitor (even more simple version) ......................................................................
Patch Set 10: Code-Review-1 (3 comments) http://gerrit.ovirt.org/#/c/36197/10/lib/vdsm/netlink/__init__.py File lib/vdsm/netlink/__init__.py: Line 66: _pool = NLSocketPool(_POOL_SIZE) Line 67: Line 68: Line 69: def _open_socket(seq_check=True, callback_function=None): Line 70: """Returns an open netlink socket.""" please document the args and return value of the callback function Line 71: sock = _nl_socket_alloc() Line 72: if sock is None: Line 73: raise IOError(get_errno(), 'Failed to allocate netlink handle') Line 74: http://gerrit.ovirt.org/#/c/36197/10/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 77: self._epoll.register(fd, select.EPOLLIN) Line 78: try: Line 79: while True: Line 80: events = NoIntrPoll(self._epoll.poll) Line 81: if (self._pipetrick[0], select.POLLIN) in events: fix comment: we do not know if "we" or someone else called stop(). All we know is that stop() was called - and we must stop. Also, for cleanliness, we should read a single char off the pipe. Line 82: # after yield we called stop, we are not interested Line 83: # in new events Line 84: break Line 85: Line 97: finally: Line 98: _close_socket(sock) Line 99: Line 100: def stop(self): Line 101: # after stop, we have to reinit Monitor Why? Why would a repeated iter() call fail? Line 102: os.write(self._pipetrick[1], 'c') Line 103: Line 104: Line 105: # libnl/include/linux/rtnetlink.h -- 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: 10 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
