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

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(a)redhat.com&gt; > >> --- > >> 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.