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

Ján Tomko jtomko at redhat.com
Fri Nov 20 10:30:44 UTC 2015

On Thu, Nov 19, 2015 at 06:15:39PM +0100, Peter Krempa wrote:
> 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".

What I meant to say was: this one is a real error, not a false positive.

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

Do you have any statistics to support that claim? I see more than one
useful commit with CHECKED_RETURN in the description.

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

'Resolve' was the keyword. I do not expect you to fix the whole driver,
but the commit message as-is did not even acknowledge that Coverity is
right here.

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

Yes, vbox driver is terrible at handling errors. Any step to improve
that is welcome. IMO this patch goes in the other direction.

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

Even with ATTRIBUTE_RETURN_CHECK turning this from a static analysis
error to a compilation error, I would rather see a partial attempt at
error recovery than an 'ignore_value'.

-------------- 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/20151120/fd26b08c/attachment-0001.sig>

More information about the libvir-list mailing list