[libvirt] [PATCH 0/3] Misc fixes to QEMU driver PCI hotplug handling
In testing PCI hotplug I found a few problems - The security driver would overwrite the real error message with a useless one of its own - We didn't correctly detect errors in the non-JSON monitor - We never re-attached the PCI dev to the host if hotplug failed to work.
Any output at all from device_add indicates an error in the command execution. Thus it needs to check for reply != "" * src/qemu/qemu_monitor_text.c: Fix reply check for errors to treat any output as an error --- src/qemu/qemu_monitor_text.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 4b1e2ec..005db25 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2319,11 +2319,11 @@ int qemuMonitorTextAddDevice(qemuMonitorPtr mon, goto cleanup; } - /* If the command failed qemu prints: - * Could not add ... */ - if (strstr(reply, "Could not add ")) { + /* If the command succeeds, not output is sent. So + * any non-empty string shows an error */ + if (STRNEQ(reply, "")) { qemuReportError(VIR_ERR_OPERATION_FAILED, - _("adding %s device failed"), devicestr); + _("adding %s device failed: %s"), devicestr, reply); goto cleanup; } -- 1.6.6.1
On 05/28/2010 05:52 AM, Daniel P. Berrange wrote:
Any output at all from device_add indicates an error in the command execution. Thus it needs to check for reply != ""
* src/qemu/qemu_monitor_text.c: Fix reply check for errors to treat any output as an error --- src/qemu/qemu_monitor_text.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 4b1e2ec..005db25 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2319,11 +2319,11 @@ int qemuMonitorTextAddDevice(qemuMonitorPtr mon, goto cleanup; }
- /* If the command failed qemu prints: - * Could not add ... */ - if (strstr(reply, "Could not add ")) { + /* If the command succeeds, not output is sent. So
s/not/no/
+ * any non-empty string shows an error */ + if (STRNEQ(reply, "")) {
Does it look any better to use: if (!*reply) { At any rate, the compiler can probably optimize both variants into the same code.
qemuReportError(VIR_ERR_OPERATION_FAILED, - _("adding %s device failed"), devicestr); + _("adding %s device failed: %s"), devicestr, reply); goto cleanup; }
ACK with the spelling fix. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The restore code is done in places where errors cannot be raised, since they will overwrite over pre-existing errors. * src/security/security_selinux.c: Only warn about failures in label restore, don't report errors --- src/security/security_selinux.c | 17 ++++++++++------- 1 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 383e189..fdbd12b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -347,6 +347,9 @@ SELinuxSetFilecon(const char *path, char *tcon) return 0; } + +/* This method shouldn't raise errors, since they'll overwrite + * errors that the caller(s) are already dealing with */ static int SELinuxRestoreSecurityFileLabel(const char *path) { @@ -354,27 +357,27 @@ SELinuxRestoreSecurityFileLabel(const char *path) security_context_t fcon = NULL; int rc = -1; char *newpath = NULL; + char ebuf[1024]; VIR_INFO("Restoring SELinux context on '%s'", path); if (virFileResolveLink(path, &newpath) < 0) { - virReportSystemError(errno, - _("cannot resolve symlink %s"), path); + VIR_WARN("cannot resolve symlink %s: %s", path, + virStrerror(errno, ebuf, sizeof(ebuf))); goto err; } if (stat(newpath, &buf) != 0) { - virReportSystemError(errno, - _("cannot stat %s"), newpath); + VIR_WARN("cannot stat %s: %s", newpath, + virStrerror(errno, ebuf, sizeof(ebuf))); goto err; } if (matchpathcon(newpath, buf.st_mode, &fcon) == 0) { rc = SELinuxSetFilecon(newpath, fcon); } else { - virSecurityReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot restore selinux file label for %s"), - newpath); + VIR_WARN("cannot lookup default selinux label for %s", + newpath); } err: -- 1.6.6.1
On 05/28/2010 05:52 AM, Daniel P. Berrange wrote:
The restore code is done in places where errors cannot be raised, since they will overwrite over pre-existing errors.
* src/security/security_selinux.c: Only warn about failures in label restore, don't report errors --- src/security/security_selinux.c | 17 ++++++++++------- 1 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 383e189..fdbd12b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -347,6 +347,9 @@ SELinuxSetFilecon(const char *path, char *tcon) return 0; }
+ +/* This method shouldn't raise errors, since they'll overwrite + * errors that the caller(s) are already dealing with */ static int SELinuxRestoreSecurityFileLabel(const char *path)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
When an attempt to hotplug a PCI device to a guest fails, the device was left attached to pci-stub. It is neccessary to reset the device and then attach it to the host driver again. * src/qemu/qemu_driver.c: Reattach PCI device to host if hotadd fails --- src/qemu/qemu_driver.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7ff5542..62b4502 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7701,6 +7701,7 @@ static int qemudDomainAttachHostPciDevice(struct qemud_driver *driver, pciFreeDevice(pci); return -1; } + pci = NULL; /* activePciHostdevs owns the 'pci' reference now */ if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { if (qemuAssignDeviceHostdevAlias(vm->def, hostdev, -1) < 0) @@ -7768,8 +7769,22 @@ error: qemuDomainPCIAddressReleaseAddr(priv->pciaddrs, &hostdev->info) < 0) VIR_WARN0("Unable to release PCI address on host device"); - VIR_FREE(devstr); + pci = pciGetDevice(hostdev->source.subsys.u.pci.domain, + hostdev->source.subsys.u.pci.bus, + hostdev->source.subsys.u.pci.slot, + hostdev->source.subsys.u.pci.function); + pciDeviceListDel(driver->activePciHostdevs, pci); + + if (pciResetDevice(pci, driver->activePciHostdevs) < 0) + VIR_WARN0("Unable to reset PCI device after assign failure"); + else if (hostdev->managed && + pciReAttachDevice(pci) < 0) + VIR_WARN0("Unable to re-attach PCI device after assign failure"); + pciFreeDevice(pci); + + + VIR_FREE(devstr); VIR_FREE(configfd_name); if (configfd >= 0) close(configfd); -- 1.6.6.1
On 05/28/2010 05:52 AM, Daniel P. Berrange wrote:
When an attempt to hotplug a PCI device to a guest fails, the device was left attached to pci-stub. It is neccessary
s/neccessary/necessary/
to reset the device and then attach it to the host driver again.
* src/qemu/qemu_driver.c: Reattach PCI device to host if hotadd fails --- src/qemu/qemu_driver.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake