Petr Horáček has posted comments on this change. Change subject: netlink: event monitor (simple version) ......................................................................
Patch Set 8: (6 comments) http://gerrit.ovirt.org/#/c/36197/8/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 152: if obj_dict is not None: Line 153: msg_type = _nl_object_get_msgtype(obj) Line 154: try: Line 155: obj_dict['event'] = _EVENTS[msg_type] Line 156: queue.put(obj_dict) > strictly speaking, this should go into the "else" clause of this try block. Done Line 157: except KeyError: Line 158: logging.error('unexpected msg_type %s', msg_type) Line 159: Line 160: Line 171: self._epoll.register(self._pipetrick[0], select.POLLIN) Line 172: if groups: Line 173: self._groups = list(groups) Line 174: else: Line 175: self._groups = _KNOWN_GROUPS.keys() > why do you need the conversion to list, or the conversion of empty set to a I didn't understand correctly nl-monitor's help text, it seems we have to add some memberships. Line 176: super(MonitorThread, self).__init__() Line 177: self.daemon = True Line 178: Line 179: def run(self): Line 177: self.daemon = True Line 178: Line 179: def run(self): Line 180: _object_input_p = partial(_object_input, self._queue) Line 181: _c_object_input = CFUNCTYPE(c_void_p, c_void_p, > I remember that there was an issue with defining CFUNCTYPE out of the main There were problems with defining object and event input functions outside this thread Line 182: c_void_p)(_object_input_p) Line 183: _event_input_p = partial(_event_input, _c_object_input) Line 184: _c_event_input = CFUNCTYPE(c_int, c_void_p, c_void_p)(_event_input_p) Line 185: self.sock = _open_socket(seq_check=False, Line 190: fd = _nl_socket_get_fd(self.sock) Line 191: self._epoll.register(fd, select.EPOLLIN) Line 192: try: Line 193: while True: Line 194: array = NoIntrPoll(self._epoll.poll) > this is not an "array", but a list - and we do not really care. Done Line 195: if (self._pipetrick[0], select.POLLIN) in array: Line 196: break Line 197: else: Line 198: _nl_recvmsgs_default(self.sock) Line 198: _nl_recvmsgs_default(self.sock) Line 199: finally: Line 200: self._epoll.unregister(fd) Line 201: finally: Line 202: _drop_socket_memberships(self.sock, self._groups) > try blocks are cheap - please add another one after _add_socket_memberships Done Line 203: _close_socket(self.sock) Line 204: http://gerrit.ovirt.org/#/c/36197/8/tests/netlinkTests.py File tests/netlinkTests.py: Line 46: dummy_name = dummy.create(prefix='dummy_mon_') Line 47: dummy.remove(dummy_name) Line 48: Line 49: mon = monitor.Monitor(timeout=2) Line 50: mon.start() > I'd open a try-block here, and call mon.stop() on "finally", to make sure w Done Line 51: thread.start_new(_add_device, tuple()) Line 52: found = False Line 53: for event in mon: Line 54: try: -- 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: 8 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
