[libvirt] [PATCH 2/2] vbox: Resolve Coverity CHECKED_RETURN

[Peter Krempa]

On Thu, Nov 19, 2015 at 09:43:25 -0500, John Ferlan wrote:
> 
> 
> On 11/19/2015 09:20 AM, Ján Tomko wrote:
> > On Thu, Nov 19, 2015 at 09:06:05AM -0500, John Ferlan wrote:
> >> Other callers check return from virDomainGraphicsListenSetAddress, but
> >> vbox doesn't in it's vboxDumpDisplay. Follow other instances within vbox
> >> to just ignore the return value in the vboxDump* functions.
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/vbox/vbox_common.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> > 
> > NACK, this silences the error instead of resolving it.
> > 
> > You can achieve the same result by not running Coverity at all :)
> 
> Sure and we can ignore many other errors Coverity points out. Coverity

And we should actually do so in some cases ...

> is a tool that points out flaws. Some are real, some are false
> positives. I understand there is a dislike/distrust of the tool by some
> for various reasons, but it is useful so I don't believe not running it
> is a "viable option".

You can disable this rather useless check though. I do agree that
coverity is useful in many aspects, but this particular check is just
bothering us and complaining about stupid stuff which up until today was
false positive all the time.

> 
> IMO: It seems the change is NACK'd based more on mentioning Coverity
> than the 'usefulness' of the code checking the return status and causing

Well, you add a piece of code which is rather useless and also breaks
formatting of the code with no real reason to do so. I think there would
be less objection if you also added ATTRIBUTE_RETURN_CHECK which would
also prevent from doing the same mistake again. Otherwise somebody else
may introduce code that will make coverity whine again.

> a failure. There are some far worse examples in vbox_common.c where
> ignore_value is used in the virDump* calls on allocation failures which
> then proceed to access what could have failed. You should have also
> noted that vboxDumpDisplay is a "static void" so failure doesn't seem to
> matter to the caller.

And do you guess why? Somebody added that to silence the compiler. Yes
it's worse than this. But you are silencing a different tool. Adding
ATTRIBUTE_RETURN_CHECK and inspecting offenders is a sensible thing to
do. Without that, it will possibly happen again.

I also dislike mentioning the coverity as the main reason to do a
change. I think it is a false claim and the change itself should be
justifiable without mentioning any tool.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151119/bf2ecb91/attachment-0001.sig>