Re: [PATCH 1/2] qemu: fix address allocation on chardev attach

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]