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

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