[libvirt] [PATCH] qemu: Resolve Coverity UNINIT

[John Ferlan]

On 11/16/2014 04:15 AM, Peter Krempa wrote:
> On 11/15/14 14:20, John Ferlan wrote:
>> For some reason, commit id '72b4151f' triggered a Coverity uninitialized
>> 'reply' variable check when referenced within the for loop.
>>
>> It seems Coverity doesn't know that flags will have to be either AFFECT_LIVE
>> or AFFECT_CONFIG after the virDomainLiveConfigHelperMethod call.
>>
>> By adding a "sa_assert()" to confirm that fact, Coverity is happy again.
> 
> Hmm, I remember few of those in the last time.
> 
> 

right - sa_assert is a different way to say, it's a false positive, but
let's let coverity (or tools like coverity) know that we expect a
specific value to be return from some call. Many times it's whether a
pointer is valid or not from some call.  The alternative is a comment
tag /* coverity[message] */ before where the usage occurs.

>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>
>>  NOTE: 
>>  Using "sa_assert(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG));
>>  did not clear the error - only the two separate checks using the ||.
>>
>>  Yes, the parentheses are overkill...
>>
>>  src/qemu/qemu_driver.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 9b19629..a84fd47 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17000,6 +17000,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
>>      if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags,
>>                                          &persistentDef) < 0)
> 
> Did you try to put the sa_assert() test inside the
> 'virDomainLiveConfigHelperMethod' so that we wouldn't need to put it in
> all the places that call the function? Would coverity be able to pass
> the result in such case?
> 

Yes, but that didn't help - it's not the called routine, it's the caller
that has two conditions, one for LIVE and one for CONFIG that return
"reply". Since "reply" is used unconditionally later, Coverity gets
suspicious.

What's really odd (to me) in this case is Coverity got suspicious when
the 'device = qemuDiskPathToAlias(vm, disk, NULL);' got moved inside the
if "LIVE" condition.

>>          goto endjob;
>> +    sa_assert((flags & VIR_DOMAIN_AFFECT_LIVE) ||
>> +              (flags & VIR_DOMAIN_AFFECT_CONFIG));
>>  
>>      if (flags & VIR_DOMAIN_AFFECT_LIVE) {
>>          /* If the VM is running, we can check if the current VM can use
>>
> 
> ACK if coverity won't be able to process this in case the assertion is
> verified inside the function that manipulates flags.
> 
> Peter
>