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>
>> ---
>> 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.
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)