[libvirt] [PATCH 0/3] qemu: Improve USB devices list handling
This patch set fixes two bugs: https://bugzilla.redhat.com/show_bug.cgi?id=806449 https://bugzilla.redhat.com/show_bug.cgi?id=743671 tl;dr - we are only adding to the list, removing only on detach-device. Need to remove on qemuProcessStop too. And need to rebuild the list on qemuProcessReconnect. Michal Privoznik (3): qemu: Don't leak temporary list of USB devices qemu: Delete USB devices used by domain on stop qemu: Build activeUsbHostdevs list on process reconnect src/qemu/qemu_hostdev.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hostdev.h | 2 + src/qemu/qemu_process.c | 3 + 3 files changed, 113 insertions(+), 4 deletions(-) -- 1.7.8.5
and add debug message when adding USB device to the list of active devices. --- src/qemu/qemu_hostdev.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 329e3fc..d4d7461 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -559,10 +559,7 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, hostdev->source.subsys.u.usb.product); if (!usb) - return -1; - - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); + goto cleanup; if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) { const char *other_name = usbDeviceGetUsedBy(tmp); @@ -579,6 +576,9 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, goto cleanup; } + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); + if (usbDeviceListAdd(list, usb) < 0) { usbFreeDevice(usb); goto cleanup; @@ -594,6 +594,9 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, for (i = 0; i < usbDeviceListCount(list); i++) { tmp = usbDeviceListGet(list, i); usbDeviceSetUsedBy(tmp, name); + + VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", + usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name); if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) { usbFreeDevice(tmp); goto inactivedevs; -- 1.7.8.5
On Mon, Mar 26, 2012 at 17:39:59 +0200, Michal Privoznik wrote:
and add debug message when adding USB device to the list of active devices. --- src/qemu/qemu_hostdev.c | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-)
ACK Jirka
To prevent assigning one USB device to two domains, we keep a list of assigned USB devices. On domain startup - qemuProcessStart() - we insert devices used by domain into the list but remove them only on detach-device. Devices are, however, released on qemuProcessReconnect() as well. --- src/qemu/qemu_hostdev.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 61 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index d4d7461..6ce2421 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -755,6 +755,64 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, pciDeviceListFree(pcidevs); } +static void +qemuDomainReAttachHostUsbDevices(struct qemud_driver *driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i; + + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + usbDevice *usb, *tmp; + const char *used_by = NULL; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) + continue; + + usb = usbGetDevice(hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); + + if (!usb) { + VIR_WARN("Unable to reattach USB device %03d.%03d on domain %s", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device, + name); + continue; + } + + /* Delete only those USB devices which belongs + * to domain @name because qemuProcessStart() might + * have failed because USB device is already taken. + * Therefore we want to steal only those devices from + * the list which were taken by @name */ + + tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb); + if (!tmp) { + VIR_WARN("Unable to find device %03d.%03d " + "in list of active USB devices", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); + usbFreeDevice(usb); + continue; + } + + used_by = usbDeviceGetUsedBy(tmp); + if (STREQ_NULLABLE(used_by, name)) { + VIR_DEBUG("Removing %03d.%03d dom=%s from activeUsbHostdevs", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device, + name); + + usbDeviceListDel(driver->activeUsbHostdevs, tmp); + } + + usbFreeDevice(usb); + } +} void qemuDomainReAttachHostDevices(struct qemud_driver *driver, virDomainDefPtr def) @@ -764,4 +822,7 @@ void qemuDomainReAttachHostDevices(struct qemud_driver *driver, qemuDomainReAttachHostdevDevices(driver, def->name, def->hostdevs, def->nhostdevs); + + qemuDomainReAttachHostUsbDevices(driver, def->name, def->hostdevs, + def->nhostdevs); } -- 1.7.8.5
On Mon, Mar 26, 2012 at 17:40:00 +0200, Michal Privoznik wrote:
To prevent assigning one USB device to two domains, we keep a list of assigned USB devices. On domain startup - qemuProcessStart() - we insert devices used by domain into the list but remove them only on detach-device. Devices are, however, released on qemuProcessReconnect() as well. ^^ qemuProcessStop() --- src/qemu/qemu_hostdev.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 61 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index d4d7461..6ce2421 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -755,6 +755,64 @@ void qemuDomainReAttachHostdevDevices(struct qemud_driver *driver, pciDeviceListFree(pcidevs); }
+static void +qemuDomainReAttachHostUsbDevices(struct qemud_driver *driver, + const char *name, + virDomainHostdevDefPtr *hostdevs, + int nhostdevs) +{ + int i; + + for (i = 0; i < nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = hostdevs[i]; + usbDevice *usb, *tmp; + const char *used_by = NULL; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) + continue; + + usb = usbGetDevice(hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); + + if (!usb) { + VIR_WARN("Unable to reattach USB device %03d.%03d on domain %s", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device, + name); + continue; + } + + /* Delete only those USB devices which belongs + * to domain @name because qemuProcessStart() might + * have failed because USB device is already taken. + * Therefore we want to steal only those devices from + * the list which were taken by @name */ + + tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb);
This is the last usage of usb so I think it's better to call usbFreeDevice(usb); here to avoid having to call it several times later.
+ if (!tmp) { + VIR_WARN("Unable to find device %03d.%03d " + "in list of active USB devices", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); + usbFreeDevice(usb); Now, the above line may be removed. + continue; + } + + used_by = usbDeviceGetUsedBy(tmp); + if (STREQ_NULLABLE(used_by, name)) { + VIR_DEBUG("Removing %03d.%03d dom=%s from activeUsbHostdevs", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device, + name); + + usbDeviceListDel(driver->activeUsbHostdevs, tmp); + } + + usbFreeDevice(usb); And this one as well. + } +}
void qemuDomainReAttachHostDevices(struct qemud_driver *driver, virDomainDefPtr def) @@ -764,4 +822,7 @@ void qemuDomainReAttachHostDevices(struct qemud_driver *driver,
qemuDomainReAttachHostdevDevices(driver, def->name, def->hostdevs, def->nhostdevs); + + qemuDomainReAttachHostUsbDevices(driver, def->name, def->hostdevs, + def->nhostdevs); }
ACK Jirka
If the daemon is restarted it will loose list of active USB devices assigned to active domains. Therefore we need to rebuild this list on qemuProcessReconnect(). --- src/qemu/qemu_hostdev.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hostdev.h | 2 ++ src/qemu/qemu_process.c | 3 +++ 3 files changed, 45 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 6ce2421..b45acec 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -157,6 +157,46 @@ int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, return 0; } +int +qemuUpdateActiveUsbHostdevs(struct qemud_driver *driver, + virDomainDefPtr def) +{ + virDomainHostdevDefPtr hostdev = NULL; + int i; + + if (!def->nhostdevs) + return 0; + + for (i = 0; i < def->nhostdevs; i++) { + usbDevice *usb = NULL; + hostdev = def->hostdevs[i]; + + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) + continue; + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) + continue; + + usb = usbGetDevice(hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device); + if (!usb) { + VIR_WARN("Unable to reattach USB device %03d.%03d on domain %s", + hostdev->source.subsys.u.usb.bus, + hostdev->source.subsys.u.usb.device, + def->name); + continue; + } + + usbDeviceSetUsedBy(usb, def->name); + + if (usbDeviceListAdd(driver->activeUsbHostdevs, usb) < 0) { + usbFreeDevice(usb); + return -1; + } + } + + return 0; +} + static int qemuDomainHostdevPciSysfsPath(virDomainHostdevDefPtr hostdev, char **sysfs_path) { diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 173e4f4..371630a 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -29,6 +29,8 @@ int qemuUpdateActivePciHostdevs(struct qemud_driver *driver, virDomainDefPtr def); +int qemuUpdateActiveUsbHostdevs(struct qemud_driver *driver, + virDomainDefPtr def); int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, const char *name, const unsigned char *uuid, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0e768fe..ae1be0f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3057,6 +3057,9 @@ qemuProcessReconnect(void *opaque) goto error; } + if (qemuUpdateActiveUsbHostdevs(driver, obj->def) < 0) + goto error; + if (qemuProcessUpdateState(driver, obj) < 0) goto error; -- 1.7.8.5
On Mon, Mar 26, 2012 at 17:40:01 +0200, Michal Privoznik wrote:
If the daemon is restarted it will loose list of active
s/loose/lose/
USB devices assigned to active domains. Therefore we need to rebuild this list on qemuProcessReconnect(). --- src/qemu/qemu_hostdev.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_hostdev.h | 2 ++ src/qemu/qemu_process.c | 3 +++ 3 files changed, 45 insertions(+), 0 deletions(-)
ACK Jirka
On 26.03.2012 17:39, Michal Privoznik wrote:
This patch set fixes two bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=806449
https://bugzilla.redhat.com/show_bug.cgi?id=743671
tl;dr - we are only adding to the list, removing only on detach-device. Need to remove on qemuProcessStop too. And need to rebuild the list on qemuProcessReconnect.
Michal Privoznik (3): qemu: Don't leak temporary list of USB devices qemu: Delete USB devices used by domain on stop qemu: Build activeUsbHostdevs list on process reconnect
src/qemu/qemu_hostdev.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hostdev.h | 2 + src/qemu/qemu_process.c | 3 + 3 files changed, 113 insertions(+), 4 deletions(-)
Ping?
On 03.04.2012 15:12, Michal Privoznik wrote:
On 26.03.2012 17:39, Michal Privoznik wrote:
This patch set fixes two bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=806449
https://bugzilla.redhat.com/show_bug.cgi?id=743671
tl;dr - we are only adding to the list, removing only on detach-device. Need to remove on qemuProcessStop too. And need to rebuild the list on qemuProcessReconnect.
Michal Privoznik (3): qemu: Don't leak temporary list of USB devices qemu: Delete USB devices used by domain on stop qemu: Build activeUsbHostdevs list on process reconnect
src/qemu/qemu_hostdev.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hostdev.h | 2 + src/qemu/qemu_process.c | 3 + 3 files changed, 113 insertions(+), 4 deletions(-)
Ping?
Thanks for review; fixed and pushed. Michal
participants (2)
-
Jiri Denemark -
Michal Privoznik