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

---------------------------------------- > From: jferlan(a)redhat.com > To: libvirt-cim(a)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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvirt-cim