[libvirt] [PATCH 0/2] Fix coverity build and resolve new issue
The first patch is a Coverity build breaker... The second one popped up recently and while some may consider it noise or a false positive, it is the only call that didn't check status. Not sure why it "popped up" after the recent changes, but that does happen. John Ferlan (2): qemu: Fix build error in Coverity environment vbox: Resolve Coverity CHECKED_RETURN src/qemu/qemu_command.h | 2 +- src/vbox/vbox_common.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.5.0
Commit id '08600de37' changed the prototype to reduce an argument, but didn't adjust the ATTRIBUTE_NONNULL(11) to (10) Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 3d9f98c..bebdd27 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -87,7 +87,7 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, virBitmapPtr nodeset, size_t *nnicindexes, int **nicindexes) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(10); /* Generate '-device' string for chardev device */ int -- 2.5.0
On Thu, Nov 19, 2015 at 09:06:04AM -0500, John Ferlan wrote:
Commit id '08600de37' changed the prototype to reduce an argument, but didn't adjust the ATTRIBUTE_NONNULL(11) to (10)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, trivial. Jan
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(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 3e6ed7a..4576a07 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3408,8 +3408,9 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (netAddressUtf16) { VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, - netAddressUtf8, -1, true); + ignore_value(virDomainGraphicsListenSetAddress( + def->graphics[def->ngraphics], 0, + netAddressUtf8, -1, true)); VBOX_UTF16_FREE(netAddressUtf16); VBOX_UTF8_FREE(netAddressUtf8); } -- 2.5.0
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 :) Jan
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 3e6ed7a..4576a07 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3408,8 +3408,9 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (netAddressUtf16) { VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, - netAddressUtf8, -1, true); + ignore_value(virDomainGraphicsListenSetAddress( + def->graphics[def->ngraphics], 0, + netAddressUtf8, -1, true)); VBOX_UTF16_FREE(netAddressUtf16); VBOX_UTF8_FREE(netAddressUtf8); } -- 2.5.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 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". 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 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. John
Jan
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 3e6ed7a..4576a07 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3408,8 +3408,9 @@ vboxDumpDisplay(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) if (netAddressUtf16) { VBOX_UTF16_TO_UTF8(netAddressUtf16, &netAddressUtf8); if (STRNEQ(netAddressUtf8, "")) - virDomainGraphicsListenSetAddress(def->graphics[def->ngraphics], 0, - netAddressUtf8, -1, true); + ignore_value(virDomainGraphicsListenSetAddress( + def->graphics[def->ngraphics], 0, + netAddressUtf8, -1, true)); VBOX_UTF16_FREE(netAddressUtf16); VBOX_UTF8_FREE(netAddressUtf8); } -- 2.5.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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".
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.
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
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@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
participants (3)
-
John Ferlan -
Ján Tomko -
Peter Krempa