Re: Coverity complaints about Remus in xen-unstable

[Hongyang Yang <yanghy@cn.fujitsu.com>]

Hi IanJ, Andrew,

   Sorry for the late replay, just back from a vacation.

在 10/02/2014 12:34 AM, Andrew Cooper 写道:
> On 01/10/14 16:32, Ian Jackson wrote:
>> scan-admin@coverity.com writes ("New Defects reported by Coverity Scan for XenProject"):
>>> Please find the latest report on new defect(s) introduced to XenProject found with Coverity Scan.
>> ...
>>> ** CID 1242320:  Uninitialized scalar variable  (UNINIT)
>>> /tools/libxl/libxl.c: 859 in libxl_domain_remus_start()
>> This is a failure to set rc on some of the error paths.  It is not a
>> big problem, but it is a bug which should be fixed, in
>> libxl_domain_remus_start.
>>
>> Yang Hongyang, could you prepare a patch to fix all the error exit
>> paths from this function to correctly set rc ?  Thanks.
>
> No need - I already did a patch earlier today, which has even been
> committed.
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bc8e20414aeacdb23d183793c1d947e28c26685b

Thank you for this.

>
>>
>>
>> Then there are also a lot like this:
>>
>>> ** CID 1242321:  Unused value  (UNUSED_VALUE)
>>> /tools/libxl/libxl_remus_device.c: 216 in all_devices_setup_cb()
>> These are all:
>>
>>>>>>      Pointer "gc" returned by "libxl__ao_inprogress_gc(ao)" is never used.
>> This is because a lot of functions were introduced which say
>>      STATE_AO_GC(something)
>> but do not happen to use the gc.  This is actually perfectly normal
>> in libxl.  And the STATE_AO_GC macro says:
>>      libxl__gc *const gc __attribute__((unused)) = libxl__ao_inprogress_gc(ao)
>> So I think this is some kind of failure by Coverity.
>
> This is not a failure in the slightest.  The statement is "You have an
> unused variable, and this constitutes a potential code maintenance
> problem which you might want to see about fixing".
>
> Coverity covers coding best practice as well as bugs.  The fact that the
> programmer has indicated that the value is unused does not invalidate
> Coverities statement about it being unused.
>
> Also bear in mind that Coverities entire purpose is to second-guess what
> the programmer has actually written, and raise queries based on what
> they plausibly may have overlooked.  Blindly trusting an
> "__attribute__((unused))" to 'fix' a compiler warning would entirely
> defeat the purpose flagging code maintenance issues.
>
>
>>
>> Weirdly, although many of these uses (eg, all_devices_setup_cb at
>> libxl_remus_device.c:212) are in functions which do not use the
>> defined `ao' variable either, and ao is _not_ marked ok-to-be-unused,
>> Coverity hasn't complained about that.
>>
>> Andrew Cooper, as our Coverity modelling expert, can you comment ?
>
> 'ao' is unconditionally used in all places, as a parameter to
> libxl__ao_inprogress_gc(), used to get the gc.
>
>>
>>
>> I don't think there is actually anything wrong with having STATE_AO_GC
>> used when not needed.  It will reduce noise in future if code is added
>> which needs it, and in the meantime it's harmless.  So I think it
>> would probably be better if STATE_AO_GC declared ao
>> __attribute__((unused)) as well.
>
> I disagree.  Removing the gc could also aleivate redundant calls to
> libxl__ao_inprogress_gc() which is not inlinable as it resides in a
> different translation unit.
>
> ~Andrew
>
> .
>

-- 
Thanks,
Yang.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel