Re: Coverity complaints about Remus in xen-unstable

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

>
>
> 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