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

[Wen Congyang]

At 04/18/2011 07:41 PM, Daniel P. Berrange Write:
> 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

Sorry for introducing a build failure.

I have tested building before pushing this patch. But I did not meet any error, and
I do not notice the warning.
The default CFLAGS does not include '-Werror'.

> 
> 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;

The return value of virDomainObjUnref() should not less than 0.
If vm is unlocked in virDomainObjUnref(), the return value is 0.
It is safe to check the return value here, but it should not happen,
as we have held an extra reference when opening the monitor.

This patch has been pushed, so I will post another patch to fix the problem.

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