https://bugzilla.redhat.com/show_bug.cgi?id=1047429 The problem occurs when a host with a running guest is upgraded from pre-1.0.2 libvirt, and then a later attempt is made to hotplug a new PCI device (emulated or passthrough) to that guest without having power cycled / save-restored / migrated the guest. The hotplug fails with this error: internal error: Device alias was not set for PCI controller with index 0 required for device at address 0000:00:xx.x This happens because the pci controller newly auto-created for the status will not contain an alias because 1) assigning an alias is normally left up to the hypervisor driver to do at domain startup, or controller attach time. 2) under the older libvirt, the PCI controller was implicit, so it was never assigned an alias. 3) when libvirtd is restarted and re-attaches to each domain, there is no "re-attach device" operation, so the no-longer-implicit PCI controller will not get an alias assigned either. When explicit PCI controllers were first added (shortly before libvirt 1.0.2), the code was written such that each controller would get an alias named "pciN" placed in its status, but the commandlines would all ignore that and instead use "pci.N". This was changed in commit 01b8812 so that 1) the proper "pci.N" alias was stored in the status, 2) the alias name from the status was used when creating the qemu commandline, and 3) when a PCI device was being hotplugged, if the status for its PCI controller had no alias assigned, an internal error would be generated. This patch changes that error condition to instead make a temporary string to contain the required alias "pci.N" and put that in the proper place in the commandline. It does not fill in the alias in the status object just in case that might create some other problem (it didn't seem very polite for the qemuBuildDeviceAddressStr() for one device to be modifying the status object for a different device). Note that this does *not* address a similar problem that would occur when libvirt was upgraded from a version between 1.0.2 and 1.1.2 to a newer version - in these cases the status would contain "pciN" as the alias name, so qemuBuildDeviceAddressStr() would be successful, but the resulting command would fail (since the qemu process has no controller named "pciN", but instead has one named "pci.N"); the problem can anyway be worked around by restarting / save-restoring / migrating the domain, and solving this second problem would require special-casing a particular alias name pattern rather than just handling a missing name, which seems just a bit too ugly to have permanently cluttering the code. --- (Yes, another exceedingly long commit log message for a very short patch :-) I'm not 100% convinced that it's necessary to handle *this* case either (upgrading from pre-1.0.2), but thought I should send the patch to the list anyway to see what others think. My opinion is that it's creating permanent clutter in the code in order to handle a situation that will come up just once in the entire lifetime of a host (and even then only if libvirt is upgraded without somehow restarting a guest, and *then* a device is hotplugged to that guest), so it may be better if we just left the workaround ("restart/save-restore/migrate the domain if you need to hotplug a device") as a footnote somewhere instead. src/qemu/qemu_command.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d723dc8..a562e18 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1,7 +1,7 @@ /* * qemu_command.c: QEMU command generation * - * Copyright (C) 2006-2013 Red Hat, Inc. + * Copyright (C) 2006-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -3021,6 +3021,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, { int ret = -1; char *devStr = NULL; + char *tempAlias = NULL; if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { const char *contAlias = NULL; @@ -3035,12 +3036,16 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, cont->idx == info->addr.pci.bus) { contAlias = cont->info.alias; if (!contAlias) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Device alias was not set for PCI " - "controller with index %u required " - "for device at address %s"), - info->addr.pci.bus, devStr); - goto cleanup; + /* this could be a case where the domain had been + * started with an earlier version of libvirt that + * had no device entry for the PCI bus + * (pre-1.0.2), and in that case no alias would be + * set in the status XML, so we create one using + * the standard "pci.N" pattern. + */ + if (virAsprintf(&tempAlias, "pci.%u", info->addr.pci.bus) < 0) + goto cleanup; + contAlias = tempAlias; } break; } @@ -3118,6 +3123,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, ret = 0; cleanup: VIR_FREE(devStr); + VIR_FREE(tempAlias); return ret; } -- 1.8.4.2 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list