[Libvirt-cim] [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params()

[John Ferlan]

On 05/17/2013 02:19 AM, WangXu wrote:
> ----------------------------------------
>> From: jferlan at redhat.com
>> To: libvirt-cim at redhat.com
>> Date: Thu, 16 May 2013 10:57:46 -0400
>> Subject: [Libvirt-cim] [PATCH 11/19] Coverity: Resolve NO_EFFECT - set_proc_rasd_params()
>>
>> 143 if (domain_online(dom))
>> 144 count = domain_vcpu_count(dom);
>> 145 else
>> 146 count = dev->dev.vcpu.quantity;
>> 147
>>
>> (1) Event unsigned_compare:
>> This greater-than-or-equal-to-zero comparison of an unsigned value
>> is always true. "count>= 0UL".
>>
>> 148 if (count>= 0)
>>
>> Resolve by adjusting logic. Problem was that the active count is returned
>> as an int with an error value of -1, while the quantity value is guaranteed
>> to be 1 or more (see parse_vcpu_device() processing). So initialize count to
>> zero, then only set the property if count> 0. Setting count of the active
>> condition requires a local "active_count" and checking that to be> 0 before
>> blindly setting it to count. Imagine 0xfffffffffffffff vcpu's!
>> ---
>> src/Virt_RASD.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c
>> index ad1a2e7..a4cba5b 100644
>> --- a/src/Virt_RASD.c
>> +++ b/src/Virt_RASD.c
>> @@ -124,7 +124,7 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker,
>> struct infostore_ctx *info = NULL;
>> uint32_t weight = 0;
>> uint64_t limit;
>> - uint64_t count;
>> + uint64_t count = 0;
>>
>> conn = connect_by_classname(broker, CLASSNAME(ref), &s);
>> if (conn == NULL)
>> @@ -140,12 +140,15 @@ static CMPIStatus set_proc_rasd_params(const CMPIBroker *broker,
>> goto out;
>> }
>>
>> - if (domain_online(dom))
>> - count = domain_vcpu_count(dom);
>> - else
>> + if (domain_online(dom)) {
>> + int active_count = domain_vcpu_count(dom);
>> + if (active_count> 0)
>> + count = active_count;
>> + } else {
>> count = dev->dev.vcpu.quantity;
>> + }
> if there was some failure happened, domain_vcpu_count would return -1. I think some
> code should handle this failure instead of just ignore it and skip CMSetProperty, Such
> as cu_statusf and goto out;
> My suggestion is the code like this:
> 
> if (domain_online(dom)) {
> int active_count = domain_vcpu_count(dom);
> if (active_count < 0) {  //count never be less than zero and maybe err code -2, -3...would
>                          //be added someday.
> cu_status(...);
> goto out;
> } else {
> count = active_count;    //equal or more than zero should be OK
> }
> } else {
> count = dev->dev.vcpu.quantity;
> }
> 
> Xu Wang
>>
>> - if (count>= 0)
>> + if (count> 0)
>> CMSetProperty(inst,
>> "VirtualQuantity",
>> (CMPIValue *)&count,
>> --
>> 1.8.1.4
>>
>> _______________________________________________
>> Libvirt-cim mailing list
>> Libvirt-cim at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvirt-cim 		 	   		  

I will squash in the following:

diff --git a/src/Virt_RASD.c b/src/Virt_RASD.c
index a4cba5b..af6a43f 100644
--- a/src/Virt_RASD.c
+++ b/src/Virt_RASD.c
@@ -142,8 +142,14 @@ static CMPIStatus set_proc_rasd_params(const
CMPIBroker *br

         if (domain_online(dom)) {
                 int active_count = domain_vcpu_count(dom);
-                if (active_count > 0)
-                        count = active_count;
+                if (active_count < 0) {
+                    cu_statusf(broker, &s,
+                               CMPI_RC_ERR_FAILED,
+                               "Unable to get domain `%s' vcpu count",
+                               domain);
+                    goto out;
+                }
+                count = active_count;
         } else {
                 count = dev->dev.vcpu.quantity;
         }



NOTE: No need for an else to if (active_count < 0) since we're jumping
to out

John