I'd rather be more specific in the commit summary, e.g.: fix address allocation on chardev hotplug On Wed, Jun 10, 2015 at 10:49:37PM +0800, Luyao Huang wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1230039 > > When attach a channel which target type is virtio, we will > get an error: > > error: Failed to attach device from channel-file.xml > error: internal error: virtio serial device has invalid address type > > This issue was introduced in commit 9807c4. > We didn't check the chr device type is serial then check > if the device target type is pci-serial, but not all the > Chr device is a serial type so we should check the device > type before check the target type to avoid assign wrong > address to other device type chr device. > > Also most of chr device do not need {pci, virtio-serial} address > in qemu, we just get the address for the device which needed. > > Signed-off-by: Luyao Huang <lhuang@xxxxxxxxxx> > --- > src/qemu/qemu_hotplug.c | 72 +++++++++++++++++++++++++++++++------------------ > 1 file changed, 46 insertions(+), 26 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 3562de6..4d60513 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1531,6 +1531,48 @@ 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 && > + virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, > + &chr->info, true) < 0) > + return -1; > + > + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && > + chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI && > + (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE || > + chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) && Do we need to check the address type here? pci-serial should always have a PCI address. > + virDomainPCIAddressEnsureAddr(priv->pciaddrs, &chr->info) < 0) > + return -1; > + > + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && > + chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO && > + virDomainVirtioSerialAddrAutoAssign(NULL, priv->vioserialaddrs, > + &chr->info, false) < 0) > + return -1; > + > + return 0; > +} > + > +static void > +qemuDomainAttachChrDeviceReleaseAddr(qemuDomainObjPrivatePtr priv, > + virDomainObjPtr vm, > + virDomainChrDefPtr chr) > +{ > + if ((chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && > + chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) || > + (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && > + chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO)) > + virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &chr->info); > + > + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL && > + chr->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_PCI) > + qemuDomainReleaseDeviceAddress(vm, &chr->info, NULL); > +} > + > int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > virDomainChrDefPtr chr) > @@ -1541,7 +1583,6 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, > 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,22 +1593,8 @@ 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; > - } > + if (qemuDomainAttachChrDeviceAssignAddr(priv, chr) < 0) > + goto cleanup; > need_release = true; > > if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0) > @@ -1601,15 +1628,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) > + qemuDomainAttachChrDeviceReleaseAddr(priv, vm, chr); Instead of creating a new function for releasing chardev addrs, it would be nicer to extend qemuDomainReleaseDeviceAddress to handle virtio-serial addresses, and only call qemuDomainReleaseDeviceAddress if we did allocate the address. Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list