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