Dan Kenigsberg has posted comments on this change. Change subject: netlink: event monitor ......................................................................
Patch Set 22: (7 comments) http://gerrit.ovirt.org/#/c/36197/22/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 38: # If monitoring thread is running, queue waiting for new value and we call Line 39: # stop(), we have to stop queue by sending special code. Line 40: _STOP_FLAG = 31 Line 41: Line 42: E_NOT_RUNNING = (1, 'Monitor is not running.') No real reason to add English text. It also complicates your exception handling. Line 43: Line 44: Line 45: class MonitorError(Exception): Line 46: pass Line 109: Line 110: def stop(self): Line 111: if self._scan_thread.is_alive(): Line 112: self._scan_thread_scanning.wait() Line 113: os.write(self._pipetrick[1], 'c') There's still a race, I'm afraid. After _scan's _pipetrick.__exit__, the fd is already BAD, but the thread is still running (although for a short while) Personally, I find events way more confusing than locks. So in this case, I think that protecting write/close with a mutex as the simplest solution. Line 114: self._scan_thread.join() Line 115: else: Line 116: raise MonitorError(E_NOT_RUNNING) Line 117: http://gerrit.ovirt.org/#/c/36197/22/tests/netlinkTests.py File tests/netlinkTests.py: Line 27: """Tests if monitor is able to catch event while iterating. Before the Line 28: iteration we start _add_device, which is delayed for .2 second. Then Line 29: iteration starts and waits for new dummy.""" Line 30: def _add_device(): Line 31: time.sleep(.2) here is a place to use threading.Event(). wait() for it here, and set() inside the "for event in mon" loop. Line 32: dummy_name = dummy.create(prefix='dummy_mon_') Line 33: dummy.remove(dummy_name) Line 34: add_device_thread = threading.Thread(target=_add_device) Line 35: Line 35: Line 36: with _timed_monitor(1) as mon: Line 37: add_device_thread.start() Line 38: for event in mon: Line 39: if 'dummy_mon_' in event.get('name'): why aren't you waiting for the exact dummy_name ? Line 40: break Line 41: add_device_thread.join() Line 42: Line 43: @ValidateRunningAsRoot Line 145: Line 146: for event in mon: Line 147: if _is_dictionary_subset(expected_events[0], event): Line 148: del expected_events[0] Line 149: if len(expected_events) == 0: How about: expected = q.get_nowait() for event in mon: if _is_dictionary_subset(expected, event): expected = q.get_nowait() ... assertEqual(q.empty()) Line 150: break Line 151: Line 152: self.assertEqual(0, len(expected_events), '%d expected events has not' Line 153: ' been caught (in the right order): %s' Line 169: finally: Line 170: try: Line 171: mon.stop() Line 172: except monitor.MonitorError as e: Line 173: # If timer was triggered, mon.stop() was called twice and second in this point of time, the timer is no longer running. So we can test if mon was stopped before calling stop() again. This would basically render the comment in Python, which is a good idea in general. Line 174: # time it raised MonitorError(1, 'Monitor is not running'). Line 175: if e[0] == monitor.E_NOT_RUNNING: Line 176: raise monitor.MonitorError('Waiting too long for a monitor' Line 177: ' event.') Line 178: else: Line 179: raise Line 180: Line 181: Line 182: def _is_dictionary_subset(subset, superset): _is_subdict() is a clearer, shorter name. -- 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: 22 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
