[libvirt] [PATCH] qemu: auto-create pci controller alias if missing from domain status
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
On 01/07/2014 07:53 AM, Laine Stump wrote:
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
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.
Yes, I think I could live with such a footnote. And even if we get outvoted and someone else requests the permanent code clutter, we can add that as a separate commit which gives the further justification of why we are adding it.
+ /* 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;
contAlias and tempAlias point to the same location...
} break; } @@ -3118,6 +3123,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, ret = 0; cleanup: VIR_FREE(devStr); + VIR_FREE(tempAlias);
and you then free tempAlias. That leaves contAlias pointing at stale memory. That doesn't feel right. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/07/2014 05:20 PM, Eric Blake wrote:
On 01/07/2014 07:53 AM, Laine Stump wrote:
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. Yes, I think I could live with such a footnote. And even if we get outvoted and someone else requests the permanent code clutter, we can add that as a separate commit which gives the further justification of why we are adding it.
Okay. I *think* that the bugzilla record and this email thread should be enough that anyone encountering the problem will find the solution after a quick google search. I don't know that putting it in libvirt release notes would be of any use, since the release it *should* have been noted in is long past...
+ /* 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; contAlias and tempAlias point to the same location...
It's a bit of a moot point since we're not going to push it, but... contAlias is just a const char * that normally would be a duplicate of cont->info.alias. Since cont->info.alias is NULL, I create a temporary alias in tempAlias and duplicate that instead.
} break; } @@ -3118,6 +3123,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, ret = 0; cleanup: VIR_FREE(devStr); + VIR_FREE(tempAlias);
and you then free tempAlias. That leaves contAlias pointing at stale memory. That doesn't feel right.
But by this time contAlias is already long ago out of scope.
On 01/07/2014 08:36 AM, Laine Stump wrote:
Yes, I think I could live with such a footnote. And even if we get outvoted and someone else requests the permanent code clutter, we can add that as a separate commit which gives the further justification of why we are adding it.
Okay. I *think* that the bugzilla record and this email thread should be enough that anyone encountering the problem will find the solution after a quick google search. I don't know that putting it in libvirt release notes would be of any use, since the release it *should* have been noted in is long past...
Agreed.
cleanup: VIR_FREE(devStr); + VIR_FREE(tempAlias); and you then free tempAlias. That leaves contAlias pointing at stale memory. That doesn't feel right.
But by this time contAlias is already long ago out of scope.
Ah, that's the context I was missing. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump