Change in vdsm[master]: monitor: Add udev monitor
rmohr at redhat.com
rmohr at redhat.com
Wed Feb 10 10:32:56 UTC 2016
Roman Mohr has posted comments on this change.
Change subject: monitor: Add udev monitor
......................................................................
Patch Set 7:
(8 comments)
Not everything done yet.
https://gerrit.ovirt.org/#/c/47729/7/lib/vdsm/udev/monitor.py
File lib/vdsm/udev/monitor.py:
Line 27:
Line 28: """
Line 29: ``udev`` event monitor. Usage:
Line 30:
Line 31: The monitor is a thread-save thin wrapper arount pyudev.MonitorObserver.
> thread-safe
Done
Line 32: This allows multiple callbacks for the same netlink socket. To avoid
Line 33: listening for udev events the application is not interested in, the
Line 34: monitoring thread only starts listening on the socket when the monitor is
Line 35: started and at least one subscriber is added.
Line 60: monitor.start()
Line 61: """
Line 62:
Line 63: def __init__(self):
Line 64: self._subsystems = {}
> This makes subscription handling little harder. Why not have a flat list of
Done
Line 65: self._context = Context()
Line 66: self._monitor = Monitor.from_netlink(self._context)
Line 67: self._observer = MonitorObserver(self._monitor, callback=partial(
Line 68: UdevMonitor._event_loop, self), name='udev-monitor')
Line 61: """
Line 62:
Line 63: def __init__(self):
Line 64: self._subsystems = {}
Line 65: self._context = Context()
> Do we need to keep the context? Isn't it enough to create it and pass it to
Done
Line 66: self._monitor = Monitor.from_netlink(self._context)
Line 67: self._observer = MonitorObserver(self._monitor, callback=partial(
Line 68: UdevMonitor._event_loop, self), name='udev-monitor')
Line 69: self._filter_lock = threading.Lock()
Line 64: self._subsystems = {}
Line 65: self._context = Context()
Line 66: self._monitor = Monitor.from_netlink(self._context)
Line 67: self._observer = MonitorObserver(self._monitor, callback=partial(
Line 68: UdevMonitor._event_loop, self), name='udev-monitor')
> Why do you need partial? Why not MonitorObserver(self._event_loop)?
Done
Line 69: self._filter_lock = threading.Lock()
Line 70: self._is_started = False
Line 71: self._can_start = False
Line 72:
Line 69: self._filter_lock = threading.Lock()
Line 70: self._is_started = False
Line 71: self._can_start = False
Line 72:
Line 73: def _event_loop(self, device):
> This is not a good name, since this is not an event loop (it does not loop)
Done
Line 74: subsystem = self._subsystems[device.subsystem]
Line 75: for callback in subsystem.get(device.device_type, []):
Line 76: _execute_callback(callback, device)
Line 77: if device.device_type:
Line 97: device_types = self._subsystems.get(subsystem, {})
Line 98: callbacks = device_types.get(device_type, [])
Line 99: callbacks.append(callback)
Line 100: device_types[device_type] = callbacks
Line 101: self._subsystems[subsystem] = device_types
> If we are using flat subscription list, this could be simplified to:
Done
Line 102: self._start_if_necessary()
Line 103:
Line 104: def start(self):
Line 105: self._can_start = True
Line 109:
Line 110: def _start_if_necessary(self):
Line 111: if self._can_start and not self._is_started:
Line 112: self._observer.start()
Line 113: self._is_started = True
> When unsubscribing the last filter, we can stop the observer.
Since there is no way to unsubscribe we have to call stop() explicitly when we are done
Line 114:
Line 115: def stop(self):
Line 116: """
Line 117: Stops the monitoring thread. It is guaranteed that callbacks are no
Line 128: try:
Line 129: callback(device)
Line 130: except Exception as callbackException:
Line 131: logging.error(
Line 132: 'Callback execution threw an exception: %s', callbackException)
> We like to see a traceback in this case, so this should be:
Done
--
To view, visit https://gerrit.ovirt.org/47729
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b91753424d83896fa538eb6b57f8653b6332fbb
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Roman Mohr <rmohr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list