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

[Pavel Hrdina]

On 10/31/2014 06:16 PM, John Ferlan wrote:
>
>
> 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
>

Oh, that's better solution. I'll update the patch and push it tomorrow 
if we don't get any answer about the first patch.

Thanks,

Pavel

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