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(a)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