On 06/30/2015 10:26 AM, Ján Tomko wrote: > From: Luyao Huang <lhuang@xxxxxxxxxx> > > Also check the device type when deciding what type the address should > be. Commit 9807c47 (aiming to fix another error in address allocation) > only checked the target type, but its value is different for different > device types. This resulted in an error when trying to attach > a channel with target type 'virtio': > > error: Failed to attach device from channel-file.xml > error: internal error: virtio serial device has invalid address type > > Make the logic for releasing the address dependent only on > * the address type > * whether it was allocated earlier > to avoid copying the device and target type checks. > > https://bugzilla.redhat.com/show_bug.cgi?id=1230039 > > Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx> > Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> > --- > src/qemu/qemu_command.c | 4 +++ > src/qemu/qemu_hotplug.c | 70 +++++++++++++++++++++++++++++-------------------- > 2 files changed, 46 insertions(+), 28 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 9b06a49..01c80c0 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1837,6 +1837,10 @@ qemuDomainReleaseDeviceAddress(virDomainObjPtr vm, > &info->addr.pci) < 0) > VIR_WARN("Unable to release PCI address on %s", > NULLSTR(devstr)); > + if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL && > + virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, info) < 0) > + VIR_WARN("Unable to release virtio-serial address on %s", > + NULLSTR(devstr)); > } > > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 0628964..c7c2ea4 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1531,17 +1531,51 @@ qemuDomainChrRemove(virDomainDefPtr vmdef, > return ret; > } > > +static int > +qemuDomainAttachChrDeviceAssignAddr(qemuDomainObjPrivatePtr priv, > + virDomainChrDefPtr chr) > +{ > + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && > + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) { > + if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, > + &chr->info, true) < 0) > + return -1; > + return 1; > + > + } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && > + chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { > + if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) > + return -1; > + return 1; > + > + } else if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && > + chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO) { > + if (virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, > + &chr->info, false) < 0) > + return -1; > + return 1; > + } > + > + if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL || > + chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Unsupported address type for character device")); > + return -1; > + } > + > + return 0; > +} > + > int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainChrDefPtr chr) > { > - int ret = -1; > + int ret = -1, rc; > qemuDomainObjPrivatePtr priv = vm->privateData; > virDomainDefPtr vmdef = vm->def; > char *devstr = NULL; > char *charAlias = NULL; > bool need_release = false; > - bool allowZero = false; > > if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { > virReportError(VIR_ERR_OPERATION_INVALID, "%s", > @@ -1552,23 +1586,10 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, > if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) > goto cleanup; > > - if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && > - chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) > - allowZero = true; > - > - if (chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { > - if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) > - goto cleanup; > - } else if (chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) { > - /* XXX */ > - } else { > - if (virDomainVirtioSerialAddrAutoAssign(NULL, > - priv->vioserialaddrs, > - &chr->info, > - allowZero) < 0) > - goto cleanup; > - } > - need_release = true; > + if ((rc = qemuDomainAttachChrDeviceAssignAddr(priv, chr) < 0)) Coverity wasn't too happy about the placement of the parentheses here - making things DEAD after here... Including the ability to set need_release The parens should be (priv, chr)) < 0) John > + goto cleanup; > + if (rc == 1) > + need_release = true; > > if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) > goto cleanup; > @@ -1601,15 +1622,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, > cleanup: > if (ret < 0 && virDomainObjIsActive(vm)) > qemuDomainChrInsertPreAllocCleanup(vm->def, chr); > - if (ret < 0 && need_release) { > - if (chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) { > - qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); > - } else if (chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB) { > - /* XXX */ > - } else { > - virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &chr->info); > - } > - } > + if (ret < 0 && need_release) > + qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); > VIR_FREE(charAlias); > VIR_FREE(devstr); > return ret; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list