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