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

[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


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