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

On Fri, Apr 15, 2011 at 10:36:10AM -0600, Eric Blake wrote: > 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. This last minute addition caused a build failure

cc1: warnings being treated as errors qemu/qemu_process.c: In function 'qemuProcessHandleWatchdog': qemu/qemu_process.c:436:34: error: ignoring return value of 'virDomainObjUnref', declared with attribute warn_unused_result [-Wunused-result] make[3]: *** [libvirt_driver_qemu_la-qemu_process.lo] Error 1 I think we also need this added: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d405dda..5a81265 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -433,14 +433,16 @@ qemuProcessHandleWatchdog(qemuMonitorPtr mon ATTRIBUTE_UNUSED, */ virDomainObjRef(vm); if (virThreadPoolSendJob(driver->workerPool, wdEvent) < 0) { - virDomainObjUnref(vm); + if (virDomainObjUnref(vm) < 0) + vm = NULL;

VIR_FREE(wdEvent); } } else virReportOOMError(); } - virDomainObjUnlock(vm); + if (vm) + virDomainObjUnlock(vm); if (watchdogEvent || lifecycleEvent) { qemuDriverLock(driver); Daniel