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@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list