[libvirt] [PATCH] qemu: fix cannot attach a virtio channel
lhuang
lhuang at redhat.com
Mon Jun 15 10:04:40 UTC 2015
On 06/15/2015 04:25 PM, Ján Tomko wrote:
> I'd rather be more specific in the commit summary, e.g.:
> fix address allocation on chardev hotplug
good summary, thanks for you help
> 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 at redhat.com>
>> ---
>> 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.
Yes, pci-serial should always have a PCI address, and add this check
is avoid always assign a pci address even we already specified the address
which type is not pci, in that case i think output an error will be better.
(although it is a corner case, and i notice when we start a guest with wrong
address type pci-serial will get the error, but cannot get the error
when attach
it, so i guess QE will complain about this ;) )
and after add this check the error will be:
# virsh attach-device rhel7.0 pci-serial.xml
error: Failed to attach device from pci-serial.xml
error: unsupported configuration: pci-serial requires address of pci type
>> + 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.
Right, i will fix this in next version.
Thanks a lot for your review and advise.
> Jan
Luyao
More information about the libvir-list
mailing list