[libvirt] [PATCH 09/10] libxl_migration: Resolve Coverity NULL_RETURNS

[Jim Fehlig]

John Ferlan wrote:
> On 08/27/2014 10:42 AM, Ján Tomko wrote:
>   
>> On 08/27/2014 03:51 PM, John Ferlan wrote:
>>     
>>> Coverity noted that all callers to libxlDomainEventQueue() could ensure
>>> the second parameter (event) was true before calling except this case.
>>> As I look at the code and how events are used - it seems that two events
>>> would have been sent which more than likely causes some bad stuff for
>>> the second one.  So just remove one call and let the cleanup: handle
>>> the event.  In the future if there's code between getting the event
>>> and cleanup that needs to send the event, this will have to change in
>>> order to send the event and set event = NULL (although it seems unlikely
>>> to happen).
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/libxl/libxl_migration.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>       
>>     
>>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>>> index dbb5a8f..eb65536 100644
>>> --- a/src/libxl/libxl_migration.c
>>> +++ b/src/libxl/libxl_migration.c
>>> @@ -519,7 +519,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>>>          libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
>>>          event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>>>                                           VIR_DOMAIN_EVENT_STOPPED_FAILED);
>>>       
>> This (possibly) overwrites the event created earlier.
>> But I'm not sure if the right solution is to send both events, or clear the
>> original one.
>>
>> Jan
>>
>>     
>
> Hmmm.. right
>
> Maybe a:
>
>     if (event) {
>         libxlDomainEventQueue(driver, event);
>         event = NULL;
>     }
>
> before the:
>
>     dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>   

Hrm, I wonder if it is even possible for dom to be NULL in this case? 
We are in the finish phase and have even unpaused the domain, if
requested.  I see that dom == NULL is never checked in
qemuMigrationFinish().  Perhaps the whole 'if (dom == NULL)' check can
be removed here too.

Regards,
Jim

> will at least cause the RESUME/SUSPEND events to be sent and then if the
> dom == NULL the STOPPED event would be sent as well
>
> I've copied Jim Fehlig for his opinion - since he wrote the code...
>
> John
>
>   
>>> -        libxlDomainEventQueue(driver, event);
>>>      }
>>>  
>>>   cleanup:
>>>
>>>