[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 <jferlan at 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