[libvirt] [PATCH 0/2] Couple of Coverity fixes
*** SOME BLURB HERE *** Michal Privoznik (2): virPerfReadEvent: Prefer saferead over read qemuProcessVerifyGuestCPU: Avoid coverity false positive src/qemu/qemu_process.c | 2 ++ src/util/virperf.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) -- 2.7.3
Do I really need to explain why? Well, if read() is interrupted int the middle of reading, we will never read the rest (even though it's highly unlikely as we are reading just 8 bytes). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virperf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virperf.c b/src/util/virperf.c index 9dc4e25..359a9c3 100644 --- a/src/util/virperf.c +++ b/src/util/virperf.c @@ -247,7 +247,7 @@ virPerfReadEvent(virPerfPtr perf, if (event == NULL || !event->enabled) return -1; - if (read(event->fd, value, sizeof(uint64_t)) < 0) { + if (saferead(event->fd, value, sizeof(uint64_t)) < 0) { virReportSystemError(errno, "%s", _("Unable to read cache data")); return -1; -- 2.7.3
On Thu, Mar 31, 2016 at 04:48:23PM +0200, Michal Privoznik wrote:
Do I really need to explain why?
Do you really need to ask?
Well, if read() is interrupted int the middle of reading, we will never read the rest (even though it's highly unlikely as we are reading just 8 bytes).
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virperf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Jan
We use _LAST items in enums to mark the last position in given enum. Now, if and enum is passed to switch(), compiler checks that all the values from enum occur in 'case' enumeration. Including _LAST. But coverity spots it's a dead code. And it really is. So to resolve this, we tend to put a comment just above 'case ..._LAST' notifying coverity that we know this is a dead code but we want to have it that way. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 07b9df2..e58bf16 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3953,6 +3953,8 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver, virDomainHypervTypeToString(i)); goto cleanup; break; + + /* coverity[dead_error_begin] */ case VIR_DOMAIN_HYPERV_LAST: break; } -- 2.7.3
On Thu, Mar 31, 2016 at 04:48:24PM +0200, Michal Privoznik wrote:
We use _LAST items in enums to mark the last position in given enum. Now, if and enum is passed to switch(), compiler checks that all the values from enum occur in 'case' enumeration. Including _LAST. But coverity spots it's a dead code. And it really is. So to resolve this, we tend to put a comment just above 'case ..._LAST' notifying coverity that we know this is a dead code but we want to have it that way.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 ++ 1 file changed, 2 insertions(+)
ACK. Would it be possible to somehow store a list of false positives for Coverity to ignore in libvirt.git? Like we do with tests/.valgrind.supp. That way we would not need to clutter up the code. Jan
On 31.03.2016 17:06, Ján Tomko wrote:
On Thu, Mar 31, 2016 at 04:48:24PM +0200, Michal Privoznik wrote:
We use _LAST items in enums to mark the last position in given enum. Now, if and enum is passed to switch(), compiler checks that all the values from enum occur in 'case' enumeration. Including _LAST. But coverity spots it's a dead code. And it really is. So to resolve this, we tend to put a comment just above 'case ..._LAST' notifying coverity that we know this is a dead code but we want to have it that way.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 ++ 1 file changed, 2 insertions(+)
ACK.
Would it be possible to somehow store a list of false positives for Coverity to ignore in libvirt.git? Like we do with tests/.valgrind.supp.
That way we would not need to clutter up the code.
Jan
That's what I've been wondering myself too. But frankly, I don't know the answer. John? Michal
On 03/31/2016 11:28 AM, Michal Privoznik wrote:
On 31.03.2016 17:06, Ján Tomko wrote:
On Thu, Mar 31, 2016 at 04:48:24PM +0200, Michal Privoznik wrote:
We use _LAST items in enums to mark the last position in given enum. Now, if and enum is passed to switch(), compiler checks that all the values from enum occur in 'case' enumeration. Including _LAST. But coverity spots it's a dead code. And it really is. So to resolve this, we tend to put a comment just above 'case ..._LAST' notifying coverity that we know this is a dead code but we want to have it that way.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 2 ++ 1 file changed, 2 insertions(+)
ACK.
Would it be possible to somehow store a list of false positives for Coverity to ignore in libvirt.git? Like we do with tests/.valgrind.supp.
That way we would not need to clutter up the code.
Jan
That's what I've been wondering myself too. But frankly, I don't know the answer. John?
I'd have to dig on this a bit... It is possible to ignore whole classes of errors using flags to the cov-analyze command; however, that's not the same as valgrind where you can disable based on stack patterns using "fun:" prefixes and elipses (...). The problem with disabling dead_error_begin warnings could be missing something that truly is a deadcode as opposed to this one where the reason why the deadcode shows up is because the typed switch() statement compiler magic requires the VIR_*_LAST: entry instead of going with the non typed one where the "default:" case would be used. John
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik