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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [Xen-devel] Coverity complaints about Remus in 
xen-unstable"):
> On 01/10/14 16:32, Ian Jackson wrote:
> > 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.

Oh, good.  I didn't spot that.  Thanks.

> > 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.  [...]
> 
> 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.

It does however mean that it is not useful to tell the programmeer
about it.  In other words, I entirely disagree with your analysis
which I think is bordering on the absurd.

Is there a way to fix this in Coverity's modelling or should we report
it as a false positive ?

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

The whole point of __attribute__((unused)) is for the programmer to be
able to say that it is _expected_ that the variable might not be used
and that it is unlikely to indicate any kind of `code maintenance
issue'.

As indeed in this case.

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

We do not care about that at all.  Nothing in these functions is even
slightly near a hot path.  We care much more about maintainability.

I would NAK a patch to remove all of these at-present-not-needed uses
of STATE_AO_GC.

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

Oh, yes, of course.

Thanks,
Ian.

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