[libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
Commit id 'f967e7a6' didn't place the closing parentheses quite right causing DEADCODE errors since the rc setting/comparison was wrong. Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 391190d..79338cf 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1586,7 +1586,7 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, if (qemuAssignDeviceChrAlias(vmdef, chr, -1) 0) goto cleanup; -if ((rc = qemuDomainAttachChrDeviceAssignAddr(priv, chr) 0)) +if ((rc = qemuDomainAttachChrDeviceAssignAddr(priv, chr)) 0) goto cleanup; if (rc == 1) need_release = true; -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
On Wed, Jul 01, 2015 at 06:31:07AM -0400, John Ferlan wrote: Commit id 'f967e7a6' didn't place the closing parentheses quite right causing DEADCODE errors since the rc setting/comparison was wrong. Signed-off-by: John Ferlan jfer...@redhat.com --- src/qemu/qemu_hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ACK, trivial. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
Coverity notes taht the switch() used to check 'connected' values has
two DEADCODE paths (_DEFAULT _LAST). Since 'connected' is a boolean
it can only be one or the other (CONNECTED or DISCONNECTED), so it just
seems pointless to use a switch to get all values. Convert to if-else
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31cbccb..1c72844 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4497,8 +4497,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
goto endjob;
if (STREQ_NULLABLE(dev.data.chr-target.name, org.qemu.guest_agent.0)) {
-switch (newstate) {
-case VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED:
+if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {
if (!priv-agent) {
if ((rc = qemuConnectAgent(driver, vm)) == -2)
goto endjob;
@@ -4506,20 +4505,13 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
if (rc 0)
priv-agentError = true;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED:
+} else {
if (priv-agent) {
qemuAgentClose(priv-agent);
priv-agent = NULL;
priv-agentError = false;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
-case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
-break;
-};
+}
if ((event = virDomainEventAgentLifecycleNewFromObj(vm, newstate,
VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL)))
--
2.1.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
On 27.04.2015 13:33, John Ferlan wrote:
Coverity notes taht the switch() used to check 'connected' values has
s/taht/that/
two DEADCODE paths (_DEFAULT _LAST). Since 'connected' is a boolean
it can only be one or the other (CONNECTED or DISCONNECTED), so it just
seems pointless to use a switch to get all values. Convert to if-else
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31cbccb..1c72844 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4497,8 +4497,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
goto endjob;
if (STREQ_NULLABLE(dev.data.chr-target.name, org.qemu.guest_agent.0))
{
-switch (newstate) {
-case VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED:
+if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {
if (!priv-agent) {
if ((rc = qemuConnectAgent(driver, vm)) == -2)
goto endjob;
@@ -4506,20 +4505,13 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
if (rc 0)
priv-agentError = true;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED:
+} else {
if (priv-agent) {
qemuAgentClose(priv-agent);
priv-agent = NULL;
priv-agentError = false;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
-case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
-break;
Can't we just put coverity[dead_error_begin] or something similar here?
-};
+}
if ((event = virDomainEventAgentLifecycleNewFromObj(vm, newstate,
VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL)))
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
On 04/27/2015 07:51 AM, Michal Privoznik wrote:
On 27.04.2015 13:33, John Ferlan wrote:
Coverity notes taht the switch() used to check 'connected' values has
s/taht/that/
two DEADCODE paths (_DEFAULT _LAST). Since 'connected' is a boolean
it can only be one or the other (CONNECTED or DISCONNECTED), so it just
seems pointless to use a switch to get all values. Convert to if-else
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31cbccb..1c72844 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4497,8 +4497,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
goto endjob;
if (STREQ_NULLABLE(dev.data.chr-target.name,
org.qemu.guest_agent.0)) {
-switch (newstate) {
-case VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED:
+if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {
if (!priv-agent) {
if ((rc = qemuConnectAgent(driver, vm)) == -2)
goto endjob;
@@ -4506,20 +4505,13 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
if (rc 0)
priv-agentError = true;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED:
+} else {
if (priv-agent) {
qemuAgentClose(priv-agent);
priv-agent = NULL;
priv-agentError = false;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
-case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
-break;
Can't we just put coverity[dead_error_begin] or something similar here?
We could... That would avoid the message. Of course there are those
that dislike the coverity markers... I did consider that, but with
'newstate' being a bool I felt how could anything be more than one or
the other. If one looks at how this is generated one ends up in
qemuMonitorJSONHandleSerialChange where virJSONValueObjectGetBoolean
fills in the 'connected' boolean which is eventually sent to
'domainSerialChange' which fills in the processEvent-action value.
While I understand the goal of using the switch() to ensure no new
virDomainChrDeviceState options cause some sort of issue - for this
particular value - a *lot* would have to change (including the qemu API)
in order for this particular one to need a switch.
Considering other approaches - using the coverity marker, one would have
to adjust the code as follows because it's multiple conditions:
/* coverity[dead_error_begin] */
case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
break;
/* coverity[dead_error_begin] */
case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
break;
};
Alternatively, see virDomainDefCheckABIStability for a slightly
different approach using #if !STATIC_ANALYSIS where multiple dead
cases are combined into one break.
Doesn't matter to me which way to go on this. Pick a preferred
mechanism. To me the if-else was least ugly.
John
-};
+}
if ((event = virDomainEventAgentLifecycleNewFromObj(vm, newstate,
VIR_CONNECT_DOMAIN_EVENT_AGENT_LIFECYCLE_REASON_CHANNEL)))
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
On Mon, Apr 27, 2015 at 07:33:06 -0400, John Ferlan wrote: Coverity notes taht the switch() used to check 'connected' values has two DEADCODE paths (_DEFAULT _LAST). Since 'connected' is a boolean it can only be one or the other (CONNECTED or DISCONNECTED), so it just seems pointless to use a switch to get all values. Convert to if-else Well, if you add a new state to the VIR_DOMAIN_CHR_DEVICE_STATE_* enum, the compiler won't warn you that you need to check yet another place if you use the if-else statement. That is the exact reason to use switch here. I hate coverity for this and I hope it will not bite me once I'll have to touch the code. Against-will ACK. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
On 27.04.2015 14:22, John Ferlan wrote:
On 04/27/2015 07:51 AM, Michal Privoznik wrote:
On 27.04.2015 13:33, John Ferlan wrote:
Coverity notes taht the switch() used to check 'connected' values has
s/taht/that/
two DEADCODE paths (_DEFAULT _LAST). Since 'connected' is a boolean
it can only be one or the other (CONNECTED or DISCONNECTED), so it just
seems pointless to use a switch to get all values. Convert to if-else
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31cbccb..1c72844 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4497,8 +4497,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
goto endjob;
if (STREQ_NULLABLE(dev.data.chr-target.name,
org.qemu.guest_agent.0)) {
-switch (newstate) {
-case VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED:
+if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {
if (!priv-agent) {
if ((rc = qemuConnectAgent(driver, vm)) == -2)
goto endjob;
@@ -4506,20 +4505,13 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
if (rc 0)
priv-agentError = true;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED:
+} else {
if (priv-agent) {
qemuAgentClose(priv-agent);
priv-agent = NULL;
priv-agentError = false;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
-case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
-break;
Can't we just put coverity[dead_error_begin] or something similar here?
We could... That would avoid the message. Of course there are those
that dislike the coverity markers... I did consider that, but with
'newstate' being a bool I felt how could anything be more than one or
the other. If one looks at how this is generated one ends up in
qemuMonitorJSONHandleSerialChange where virJSONValueObjectGetBoolean
fills in the 'connected' boolean which is eventually sent to
'domainSerialChange' which fills in the processEvent-action value.
While I understand the goal of using the switch() to ensure no new
virDomainChrDeviceState options cause some sort of issue - for this
particular value - a *lot* would have to change (including the qemu API)
in order for this particular one to need a switch.
Considering other approaches - using the coverity marker, one would have
to adjust the code as follows because it's multiple conditions:
/* coverity[dead_error_begin] */
case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
break;
/* coverity[dead_error_begin] */
case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
break;
};
Alternatively, see virDomainDefCheckABIStability for a slightly
different approach using #if !STATIC_ANALYSIS where multiple dead
cases are combined into one break.
Doesn't matter to me which way to go on this. Pick a preferred
mechanism. To me the if-else was least ugly.
Okay. Let's go that way then.
BTW: as Coverity evolves, do we revisit all the false positives we have
in our code? I mean, some of them may be no longer needed. Or maybe the
surrounding code changes so coverity would not report any false
positive. Just asking whether you (or somebody else) considered that. I
can imagine how terribly boring can that be.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE
On 04/27/2015 11:22 AM, Michal Privoznik wrote:
On 27.04.2015 14:22, John Ferlan wrote:
On 04/27/2015 07:51 AM, Michal Privoznik wrote:
On 27.04.2015 13:33, John Ferlan wrote:
Coverity notes taht the switch() used to check 'connected' values has
s/taht/that/
two DEADCODE paths (_DEFAULT _LAST). Since 'connected' is a boolean
it can only be one or the other (CONNECTED or DISCONNECTED), so it just
seems pointless to use a switch to get all values. Convert to if-else
Signed-off-by: John Ferlan jfer...@redhat.com
---
src/qemu/qemu_driver.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 31cbccb..1c72844 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4497,8 +4497,7 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
goto endjob;
if (STREQ_NULLABLE(dev.data.chr-target.name,
org.qemu.guest_agent.0)) {
-switch (newstate) {
-case VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED:
+if (newstate == VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED) {
if (!priv-agent) {
if ((rc = qemuConnectAgent(driver, vm)) == -2)
goto endjob;
@@ -4506,20 +4505,13 @@ processSerialChangedEvent(virQEMUDriverPtr driver,
if (rc 0)
priv-agentError = true;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED:
+} else {
if (priv-agent) {
qemuAgentClose(priv-agent);
priv-agent = NULL;
priv-agentError = false;
}
-break;
-
-case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
-case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
-break;
Can't we just put coverity[dead_error_begin] or something similar here?
We could... That would avoid the message. Of course there are those
that dislike the coverity markers... I did consider that, but with
'newstate' being a bool I felt how could anything be more than one or
the other. If one looks at how this is generated one ends up in
qemuMonitorJSONHandleSerialChange where virJSONValueObjectGetBoolean
fills in the 'connected' boolean which is eventually sent to
'domainSerialChange' which fills in the processEvent-action value.
While I understand the goal of using the switch() to ensure no new
virDomainChrDeviceState options cause some sort of issue - for this
particular value - a *lot* would have to change (including the qemu API)
in order for this particular one to need a switch.
Considering other approaches - using the coverity marker, one would have
to adjust the code as follows because it's multiple conditions:
/* coverity[dead_error_begin] */
case VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT:
break;
/* coverity[dead_error_begin] */
case VIR_DOMAIN_CHR_DEVICE_STATE_LAST:
break;
};
Alternatively, see virDomainDefCheckABIStability for a slightly
different approach using #if !STATIC_ANALYSIS where multiple dead
cases are combined into one break.
Doesn't matter to me which way to go on this. Pick a preferred
mechanism. To me the if-else was least ugly.
Okay. Let's go that way then.
I assume you meant the if-else method...
BTW: as Coverity evolves, do we revisit all the false positives we have
in our code? I mean, some of them may be no longer needed. Or maybe the
surrounding code changes so coverity would not report any false
positive. Just asking whether you (or somebody else) considered that. I
can imagine how terribly boring can that be.
I haven't done that recently, although false positives generally don't
become OK at some point, unless it's a Coverity bug. We did have one
of those (for which I reported) with the VIR_FREE() mechanism... I did
get a report that it was fixed in some release, but I cannot recall if
I was able to successfully test that in our environment.
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
