Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE

[Ján Tomko]

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 
> ---
>  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

Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE

[John Ferlan]

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 
 ---
  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

Re: [libvirt] [PATCH] qemu: Resolve Coverity DEADCODE

[Michal Privoznik]

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 
>>> ---
>>>  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

[John Ferlan]

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 
>> ---
>>  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

[Michal Privoznik]

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 
> ---
>  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

[Peter Krempa]

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