Dan Kenigsberg has posted comments on this change. Change subject: netlink: event monitor ......................................................................
Patch Set 20: Code-Review-1 (11 comments) http://gerrit.ovirt.org/#/c/36197/20/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 102: self._queue.put(_STOP_FLAG) Line 103: break Line 104: _nl_recvmsgs_default(sock) Line 105: Line 106: def stop(self): as found by your EBADF comment, we have a bug here. We must never write to self._pipetrick[1] after it has been closed. The only idea I have to avoid it is to initialize self._sync and acquire it before write and close. Line 107: os.write(self._pipetrick[1], 'c') Line 108: self._scan_thread.join() Line 109: Line 110: http://gerrit.ovirt.org/#/c/36197/20/tests/netlinkTests.py File tests/netlinkTests.py: Line 11: from testValidation import ValidateRunningAsRoot Line 12: from testlib import VdsmTestCase as TestCaseBase Line 13: Line 14: Line 15: class TestNetlinkEventMonitor(TestCaseBase): -> NetlinkEventMonitorTests Line 16: Line 17: @ValidateRunningAsRoot Line 18: def testEventMonitorRunningAddDeviceBefore(self): Line 19: with _timed_monitor(1) as mon: Line 14: Line 15: class TestNetlinkEventMonitor(TestCaseBase): Line 16: Line 17: @ValidateRunningAsRoot Line 18: def testEventMonitorRunningAddDeviceBefore(self): no need to the EventMonitor prefix - all this class does is test EventMonitor. test_iterate_after_events() seems like a clearer name. Line 19: with _timed_monitor(1) as mon: Line 20: dummy_name = dummy.create(prefix='dummy_mon_') Line 21: dummy.remove(dummy_name) Line 22: for event in mon: Line 20: dummy_name = dummy.create(prefix='dummy_mon_') Line 21: dummy.remove(dummy_name) Line 22: for event in mon: Line 23: try: Line 24: if 'dummy_mon_' in event['name']: why aren't you waiting for the exact dummy_name ? I'm not sure that exception handling has any benefit here over using event.get('name'). Line 25: break Line 26: except KeyError: Line 27: pass Line 28: Line 26: except KeyError: Line 27: pass Line 28: Line 29: @ValidateRunningAsRoot Line 30: def testEventMonitorRunningAddDeviceDuring(self): test_iterate_while_events() Line 31: def _add_device(): Line 32: time.sleep(.2) Line 33: dummy_name = dummy.create(prefix='dummy_mon_') Line 34: dummy.remove(dummy_name) Line 28: Line 29: @ValidateRunningAsRoot Line 30: def testEventMonitorRunningAddDeviceDuring(self): Line 31: def _add_device(): Line 32: time.sleep(.2) why's the sleep? could you add a comment? Line 33: dummy_name = dummy.create(prefix='dummy_mon_') Line 34: dummy.remove(dummy_name) Line 35: add_device_thread = threading.Thread(target=_add_device) Line 36: Line 51: with _timed_monitor(1) as mon: Line 52: dummy.remove(dummy_name) Line 53: Line 54: found = False Line 55: for event in mon: consider using a helper function. How about: found = any(event.get(dummy_name)==dummy_name for event in mon) ? Line 56: try: Line 57: if 'dummy_mon_' in event['name']: Line 58: found = True Line 59: break Line 70: dummy.setLinkUp(dummy_name) Line 71: dummy.remove(dummy_name) Line 72: Line 73: for event in mon_a: Line 74: self.assertIn('_addr', event['event'], "Catched event '%s' is not " Catched->caught Line 75: "related to address." % event['event']) Line 76: Line 77: for event in mon_l_r: Line 78: link_or_route = ('_link' in event['event'] or Line 97: iterator.next() Line 98: Line 99: @ValidateRunningAsRoot Line 100: def testEventMonitorEventsKeys(self): Line 101: # TEMP NOTE: we don't need this test, it could be added with improved I'm not sure about the purpose of this test; but I'd love to verify that mon sniffs a complete list of events, in the right order. Line 102: # ifup which will use mechanism for testing expected events Line 103: with _timed_monitor(1) as mon: Line 104: dummy_name = dummy.create() Line 105: dummy.setIP(dummy_name, IP_ADDRESS, IP_CIDR) Line 110: {'name': dummy_name, 'type': 'dummy', 'event': 'new_link'}, Line 111: {'label': dummy_name, 'family': 'inet', 'address': IP_ADDRESS + Line 112: '/' + IP_CIDR, 'event': 'new_addr'}, Line 113: {'oif': dummy_name, 'event': 'new_route'}] Line 114: for event in mon: how about an exact list comparison - after filtering only dummy_name related events? Line 115: for expected_event in expected_events: Line 116: try: Line 117: if all([expected_event[k] == event[k] Line 118: for k in expected_events.keys()]): Line 140: try: Line 141: mon.stop() Line 142: except OSError as e: Line 143: # If timer was triggered, mon.stop() was called twice and second Line 144: # time it raised EBADF. this must never ever happen, because the second mon.stop() can happen after the relevant fd has been recycled, and who knows which file would get the stop char written to it. Line 145: if e.errno == errno.EBADF: Line 146: raise monitor.MonitorError('Waiting too long for a monitor' Line 147: ' event.') Line 148: else: -- 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: 20 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
