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

[John Ferlan]

On 11/20/2015 05:30 AM, Ján Tomko wrote:
> 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.
> 

I believe this is a question to Peter; however, from personal experience
not checking the return value is perhaps one of the more common
mistakes, especially when calling libc/gnuc functions and assuming what
you get back won't be an error (and I'm talking beyond just the 3 years
of my involvement in libvirt). Because of the recent question on list
about libvirt/glusterfs leaking memory - I ran Coverity against the
glusterfs.git tree - there are 50+ cases of this particular error and a
majority are some libc call (fcntl, getgrgid, stat, etc.)... There's
also >65 RESOURCE_LEAK's...

Anyway, in this particular case, I believe the reason the check was
triggered was because a recent commit moved where the call was made in
another module and it seems that caused Coverity to take another look at
all the callers. It found one not checking status and tagged it.

>>>
>>> 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 it did resolve the Coverity error. I am fine with the patch not
being the "desired" solution and the real solution should be to cause
the error and somehow force an exit. It's a "fine line" sometimes
between following existing practices in a particular driver and
enforcing practices of the dominant driver in other drivers, especially
ones that are less actively changed.

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

Formatting rules also don't want longer than 80 cols per line - the
format change made it more readable for me. I don't think "every"
formatting rule/norm is followed on every patch. I assume you refer to
adding ATTRIBUTE_RETURN_CHECK to virDomainGraphicsListenSetAddress.
While sure a correct thing to do - I would submit that that is a grain
of sand in the (small ;-)) beach of libvirt API's that don't have/use
that in their prototype that probably could/should. That's a deflection.
I would submit the counter argument that between current review
practices and human nature to check how other code uses a function, that
any future code that would add a call to the function would have a
return value check before the code got to master. This code was already
there - reformatted by commit id '3611c400' (Aug/2014) from commit id
'ef79fb5b' (Aug/2011)... Some of which checked return status and some
that didn't - although the changes did follow how the existing code
"handled" an error.

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

Perhaps a full-time job for a few solid weeks (make change, post change,
get review, perhaps adjust change, re-review, and finally push change)
to repair just the things that cause failures. Any one up to review a
large patch series of "fix format", "conform to current design norms",
and "add error checking", rinse repeat. Only 7878 lines in vbox_common.c
to work through. :-) Is it worth the time - tough call especially when
it comes down the unwritten rule of last person to touch owns future
problems!! Of course I could just adjust my build environment to not
compile vbox and the problem goes away in my environment!

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

Part of the reason I try post with Coverity mentioned or with the
Coverity error is proper attribution. It's not like I was combing the
sources looking for failure to check return values, although sometimes I
do find them while working in code. Likewise, if valgrind found a
problem I would want to know that it was that tool. Or even when there
were a flurry of patches as a result of running the virt-test suite (now
known as avacado). Before Coverity was used I would assume based on some
comments in the code that clang was used to find similar errors.

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

Should part of the exercise be to introspect for all potential issues
that could be hidden behind 'ignore_value'? I started with the
'sa_assert' a few months ago, but a couple were tagged with clang
comments, so I wasn't sure/comfortable with just removing the sa_assert
"just because" coverity didn't complain about it. Likewise there was one
patch where it was desired to adjust the logic to use virBitmap*
functions which I didn't end up pushing because I found no way to test
the changes didn't cause a regression even though logically they
followed existing practices in other drivers and they should work.


John