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

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list