On Thu, Nov 19, 2015 at 09:06:04AM -0500, John Ferlan wrote: ACK, trivial. Jan

On Thu, Nov 19, 2015 at 09:43:25 -0500, John Ferlan wrote: And we should actually do so in some cases ... 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. 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. 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.

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