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 <jferlan(a)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