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 <jferlan(a)redhat.com&gt; >>> --- >>> 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.