2010/1/25 Dave Allan <dallan@redhat.com>:
On 01/25/2010 02:33 PM, Matthias Bolte wrote:
2010/1/25 Dave Allan<dallan@redhat.com>:
On 01/25/2010 06:37 AM, Daniel P. Berrange wrote:
On Sun, Jan 24, 2010 at 11:07:59PM +0100, Matthias Bolte wrote:
udevDeviceMonitorStartup registers udevEventHandleCallback as event handle, but doesn't store the returned watch id to remove it later on. Also it's not clear to me whether the event handle should be register for the whole lifetime of the udev driver instance or just for the udevEnumerateDevices call.
The handler should be active for the lifetime of libvirtd, since the udev driver has to detect hotplug/unplug events over time.
If for example the call to udevSetupSystemDev [1] fails udevDeviceMonitorShutdown is called to cleanup, but udevEventHandleCallback is still registered and may be called when driverState is NULL again, resulting in a segfault in udevEventHandleCallback.
So to solve this the udevEventHandleCallback event handle must be removed at the appropriate place.
Yes, sounds like its needs to be removed in the failure path there
Matthias,
Indeed, that's correct--can you submit a patch?
Dave
Yes, im going to do that.
One last question. The udev driver stores the udev monitor handle in the private data pointer of the gloval driver state variable.
driverState->privateData = udev_monitor;
To solve the event handle problem we need to store the watch id returned by virEventAddHandle somewhere. We could either add a new private data struct to hold the udev_monitor pointer and the watch id, or store the watch id variable globally side by side with the driver state variable. I prefer the first option, because it's cleaner and the DRV_STATE_UDEV_MONITOR define allows for changing the storage location of the udev_monitor pointer easily.
Yes, I agree--a struct is the right approach. Thanks for doing this!
Dave
Here's the patch. Maximilian Wilhelm tested it. Matthias