[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] Coverity complaints about Remus in xen-unstable



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@xxxxxxxxxxxx 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.