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@xxxxxxxxxx> > > >> --- > > >> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list