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

[Hongyang Yang <yanghy@xxxxxxxxxxxxxx>]

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