[libvirt] [PATCH] qemu: fix cannot attach a virtio channel

[Ján Tomko]

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 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.

> +        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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150615/e1a85998/attachment-0001.sig>