[libvirt] [PATCH 2/2] hotplug: fix char device detach

[John Ferlan]

On 10/31/2014 12:45 PM, Pavel Hrdina wrote:
> Hotplugging and hotunplugging char devices is only supported through
> '-device' and the check for device capability should be independently.
> 
> Coverity also complains about 'tmpChr->info.alias' could be NULL and we
> are dereferencing it but it somehow only in this case don't recognize
> that the value is set by 'qemuAssignDeviceChrAlias' so it's clearly
> false positive. Add comment to skip this error in this case.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index d3bf392..520c0db 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3713,18 +3713,22 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
>          return ret;
>      }
>  
> -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE) &&
> -        !tmpChr->info.alias) {
> -        if (qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0)
> -            return ret;
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("qemu does not support -device"));
> +        return ret;
>      }

Doh!  I should have looked at the Attach code which has this check first
and done the same thing. Instead I was following the model of other
patches in my series.

You can choose to leave it there or hoist it as first check, too

>  
> +    if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0)
> +        return ret;
> +

Add 'sa_assert(tmpChr->info.alias);' see [1]

>      if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0)
>          return ret;
>  
>      qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> +    /* coverity[var_deref_model] */

[1] This could be too generic...

By adding the sa_assert() above which only gets executed under Coverity
you avoid the need for this

ACK with those changes

John

>      if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) {
>          qemuDomainObjExitMonitor(driver, vm);
>          goto cleanup;
>