Re: [libvirt] [PATCH V3 2/2] enhance processWatchdogEvent()

On 04/14/2011 09:11 PM, Wen Congyang wrote: > This patch do the following two things: s/do/does/ > 1. hold an extra reference while handling watchdog event > If the domain is not persistent, and qemu quits unexpectedly before > calling processWatchdogEvent(), vm will be freed and the function > processWatchdogEvent() will be dangerous. > > 2. unlock qemu driver and vm before returning from processWatchdogEvent() > When the function processWatchdogEvent() failed, we only free wdEvent, > but forget to unlock qemu driver and vm, free dumpfile. > > > --- > src/qemu/qemu_driver.c | 34 ++++++++++++++++++++++------------ > src/qemu/qemu_process.c | 4 ++++ > 2 files changed, 26 insertions(+), 12 deletions(-) Looks like your v2 caught my review comments correctly. But I found one more issue: > +++ b/src/qemu/qemu_process.c > @@ -428,6 +428,10 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED, > if (VIR_ALLOC(wdEvent) == 0) { > wdEvent->action = VIR_DOMAIN_WATCHDOG_ACTION_DUMP; > wdEvent->vm = vm; > + /* Hold an extra reference because we can't allow 'vm' to be > + * deleted before handling watchdog event is finished. > + */ > + virDomainObjRef(vm); > ignore_value(virThreadPoolSendJob(driver->workerPool, wdEvent)); Now that we have increased the ref count, we should decrease it if we are unable to send a job to the thread pool. That is, replace the ignore_value() with: if (virThreadPoolSendJob(...) < 0) { virDomainObjUnref(vm); VIR_FREE(wdEvent); } ACK with that change squashed in.