Now when code handling attaching/detaching usb hostdev is appropriately
changed use it to handle host usb device udev add/del events.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
src/qemu/Makefile.inc.am | 2 +
src/qemu/qemu_conf.h | 3 +
src/qemu/qemu_domain.c | 2 +
src/qemu/qemu_domain.h | 2 +
src/qemu/qemu_driver.c | 351 ++++++++++++++++++++++++++++++++++++++-
5 files changed, 359 insertions(+), 1 deletion(-)
diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
index d16b315ebc..8be0dee396 100644
--- a/src/qemu/Makefile.inc.am
+++ b/src/qemu/Makefile.inc.am
@@ -85,6 +85,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
-I$(srcdir)/conf \
-I$(srcdir)/secret \
$(AM_CFLAGS) \
+ $(UDEV_CFLAGS) \
$(NULL)
libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS)
libvirt_driver_qemu_impl_la_LIBADD = \
@@ -93,6 +94,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
$(LIBNL_LIBS) \
$(SELINUX_LIBS) \
$(LIBXML_LIBS) \
+ $(UDEV_LIBS) \
$(NULL)
libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 0cbddd7a9c..2e50bb0950 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -294,6 +294,9 @@ struct _virQEMUDriver {
/* Immutable pointer, self-locking APIs */
virHashAtomicPtr migrationErrors;
+
+ struct udev_monitor *udev_monitor;
+ int udev_watch;
};
virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 657f3ecfe4..4784804d1e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15034,6 +15034,8 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
case QEMU_PROCESS_EVENT_BLOCK_JOB:
case QEMU_PROCESS_EVENT_MONITOR_EOF:
+ case QEMU_PROCESS_EVENT_USB_REMOVED:
+ case QEMU_PROCESS_EVENT_USB_ADDED:
VIR_FREE(event->data);
break;
case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index d097f23342..94aea62693 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -521,6 +521,8 @@ typedef enum {
QEMU_PROCESS_EVENT_MONITOR_EOF,
QEMU_PROCESS_EVENT_PR_DISCONNECT,
QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
+ QEMU_PROCESS_EVENT_USB_REMOVED,
+ QEMU_PROCESS_EVENT_USB_ADDED,
QEMU_PROCESS_EVENT_LAST
} qemuProcessEventType;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2378a2e7d0..ce41b0a8d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -34,6 +34,7 @@
#include <sys/ioctl.h>
#include <sys/un.h>
#include <byteswap.h>
+#include <libudev.h>
#include "qemu_driver.h"
@@ -719,6 +720,254 @@ qemuDomainFindMaxID(virDomainObjPtr vm,
}
+struct qemuUdevUSBRemoveData {
+ unsigned int bus;
+ unsigned int device;
+};
+
+struct qemuUdevUSBAddData {
+ unsigned int vendor;
+ unsigned int product;
+};
+
+struct qemuUdevUSBEventData {
+ union {
+ struct qemuUdevUSBRemoveData remove;
+ struct qemuUdevUSBAddData add;
+ } data;
+ bool found;
+ bool remove;
+};
+
+static int
+qemuUdevUSBHandleEvent(virDomainObjPtr vm, void *opaque)
+{
+ struct qemuUdevUSBEventData *data = opaque;
+ struct qemuProcessEvent *event = NULL;
+ size_t i;
+
+ if (data->found)
+ return 0;
+
+ virObjectLock(vm);
+
+ if (!virDomainObjIsActive(vm))
+ goto cleanup;
+
+ for (i = 0; i < vm->def->nhostdevs; i++) {
+ virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i];
+ virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
+
+ if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
+ continue;
+
+ if (data->remove) {
+ if (usbsrc->bus != data->data.remove.bus ||
+ usbsrc->device != data->data.remove.device)
+ continue;
+ } else {
+ if (usbsrc->vendor != data->data.add.vendor ||
+ usbsrc->product != data->data.add.product)
+ continue;
+ }
+
+ data->found = true;
+
+ if (VIR_ALLOC(event) < 0)
+ goto cleanup;
+
+ if (data->remove) {
+ struct qemuUdevUSBRemoveData *rm_data;
+
+
+ if (VIR_ALLOC(rm_data) < 0)
+ goto cleanup;
+
+ *rm_data = data->data.remove;
+ event->data = rm_data;
+ event->eventType = QEMU_PROCESS_EVENT_USB_REMOVED;
+ } else {
+ struct qemuUdevUSBAddData *add_data;
+
+ if (VIR_ALLOC(add_data) < 0)
+ goto cleanup;
+
+ *add_data = data->data.add;
+ event->data = add_data;
+ event->eventType = QEMU_PROCESS_EVENT_USB_ADDED;
+ }
+
+ event->vm = virObjectRef(vm);
+
+ if (virThreadPoolSendJob(qemu_driver->workerPool, 0, event) < 0) {
+ virObjectUnref(vm);
+ goto cleanup;
+ }
+
+ event = NULL;
+
+ break;
+ }
+
+ cleanup:
+ virObjectUnlock(vm);
+
+ qemuProcessEventFree(event);
+
+ return 0;
+}
+
+
+static void
+qemuUdevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
+ int fd ATTRIBUTE_UNUSED,
+ int events ATTRIBUTE_UNUSED,
+ void *data ATTRIBUTE_UNUSED)
+{
+ struct qemuUdevUSBEventData event_data;
+ struct udev_device *dev = NULL;
+ const char *action;
+ const char *devtype;
+ const char *tmp;
+
+ /* libvirtd daemon do not run event loop before full state drivers
+ * initialization. Also state drivers uninitialized only after
+ * full stop of event loop. In short driver initialization/uninitialization
+ * and handling events occurs in same main loop thread. Thus we
+ * don't need any locking here. */
+
+ if (!(dev = udev_monitor_receive_device(qemu_driver->udev_monitor))) {
+ VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ VIR_WARNINGS_RESET
+ return;
+ }
+
+ virReportSystemError(errno, "%s",
+ _("failed to receive device from udev monitor"));
+ return;
+ }
+
+ devtype = udev_device_get_devtype(dev);
+
+ if (STRNEQ_NULLABLE(devtype, "usb_device"))
+ goto cleanup;
+
+ if (!(action = udev_device_get_action(dev))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to receive action from udev monitor"));
+ goto cleanup;
+ }
+
+ if (STREQ(action, "remove")) {
+ struct qemuUdevUSBRemoveData *rm_data = &event_data.data.remove;
+
+ if (!(tmp = udev_device_get_property_value(dev, "BUSNUM"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to receive busnum from udev monitor"));
+ goto cleanup;
+ }
+ if (virStrToLong_ui(tmp, NULL, 10, &rm_data->bus) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to convert busnum to int"));
+ goto cleanup;
+ }
+
+ if (!(tmp = udev_device_get_property_value(dev, "DEVNUM"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to receive devnum from udev monitor"));
+ goto cleanup;
+ }
+ if (virStrToLong_ui(tmp, NULL, 10, &rm_data->device) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to convert devnum to int"));
+ goto cleanup;
+ }
+ event_data.remove = true;
+ } else if (STREQ(action, "add")) {
+ struct qemuUdevUSBAddData *add_data = &event_data.data.add;
+
+ if (!(tmp = udev_device_get_property_value(dev, "ID_VENDOR_ID"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to receive vendor from udev monitor"));
+ goto cleanup;
+ }
+ if (virStrToLong_ui(tmp, NULL, 16, &add_data->vendor) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to convert vendor to int"));
+ goto cleanup;
+ }
+
+ if (!(tmp = udev_device_get_property_value(dev, "ID_MODEL_ID"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to receive product from udev monitor"));
+ goto cleanup;
+ }
+ if (virStrToLong_ui(tmp, NULL, 16, &add_data->product) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to convert product to int"));
+ goto cleanup;
+ }
+ event_data.remove = false;
+ }
+
+ event_data.found = false;
+ virDomainObjListForEach(qemu_driver->domains, qemuUdevUSBHandleEvent, &event_data);
+
+ cleanup:
+ udev_device_unref(dev);
+}
+
+
+static int
+qemuUdevInitialize(void)
+{
+ struct udev *udev;
+
+ if (!(udev = udev_new())) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("failed to create udev context"));
+ return -1;
+ }
+
+ if (!(qemu_driver->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"))) {
+ udev_unref(udev);
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("udev_monitor_new_from_netlink returned NULL"));
+ return -1;
+ }
+
+ udev_monitor_enable_receiving(qemu_driver->udev_monitor);
+
+ qemu_driver->udev_watch = virEventAddHandle(udev_monitor_get_fd(qemu_driver->udev_monitor),
+ VIR_EVENT_HANDLE_READABLE,
+ qemuUdevEventHandleCallback, NULL, NULL);
+
+ if (qemu_driver->udev_watch < 0)
+ return -1;
+
+ return 0;
+}
+
+
+static void
+qemuUdevCleanup(void)
+{
+ if (qemu_driver->udev_monitor) {
+ struct udev *udev = udev_monitor_get_udev(qemu_driver->udev_monitor);
+
+ udev_monitor_unref(qemu_driver->udev_monitor);
+ udev_unref(udev);
+ qemu_driver->udev_monitor = NULL;
+ }
+
+ if (qemu_driver->udev_watch > 0) {
+ virEventRemoveHandle(qemu_driver->udev_watch);
+ qemu_driver->udev_watch = 0;
+ }
+}
+
+
/**
* qemuStateInitialize:
*
@@ -1030,6 +1279,9 @@ qemuStateInitialize(bool privileged,
if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
goto error;
+ if (qemuUdevInitialize() < 0)
+ goto error;
+
/* Get all the running persistent or transient configs first */
if (virDomainObjListLoadAllConfigs(qemu_driver->domains,
cfg->stateDir,
@@ -1239,6 +1491,8 @@ qemuStateCleanup(void)
virLockManagerPluginUnref(qemu_driver->lockManager);
+ qemuUdevCleanup();
+
virMutexDestroy(&qemu_driver->lock);
VIR_FREE(qemu_driver);
@@ -5011,7 +5265,96 @@ processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
}
-static void qemuProcessEventHandler(void *data, void *opaque)
+static void
+processUSBAddedEvent(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ struct qemuUdevUSBAddData *data)
+{
+ virDomainHostdevDefPtr hostdev;
+ virDomainHostdevSubsysUSBPtr usbsrc;
+ size_t i;
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ return;
+
+ if (!virDomainObjIsActive(vm)) {
+ VIR_DEBUG("Domain is not running");
+ goto cleanup;
+ }
+
+ for (i = 0; i < vm->def->nhostdevs; i++) {
+ hostdev = vm->def->hostdevs[i];
+
+ if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
+ continue;
+
+ usbsrc = &hostdev->source.subsys.u.usb;
+
+ if (usbsrc->vendor == data->vendor && usbsrc->product == data->product)
+ break;
+ }
+
+ if (i == vm->def->nhostdevs)
+ goto cleanup;
+
+ if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
+ goto cleanup;
+
+ cleanup:
+ qemuDomainObjEndJob(driver, vm);
+}
+
+
+static void
+processUSBRemovedEvent(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ struct qemuUdevUSBRemoveData *data)
+{
+ size_t i;
+ virDomainHostdevDefPtr hostdev;
+ virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_HOSTDEV };
+
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
+ return;
+
+ if (!virDomainObjIsActive(vm)) {
+ VIR_DEBUG("Domain is not running");
+ goto cleanup;
+ }
+
+ for (i = 0; i < vm->def->nhostdevs; i++) {
+ virDomainHostdevSubsysUSBPtr usbsrc;
+
+ hostdev = vm->def->hostdevs[i];
+ if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
+ continue;
+
+ usbsrc = &hostdev->source.subsys.u.usb;
+
+ /* don't mess with devices that don't use stable host addressing
+ * with respect to unplug/plug to host
+ */
+ if (!usbsrc->vendor || !usbsrc->product)
+ continue;
+
+ if (usbsrc->bus == data->bus && usbsrc->device == data->device)
+ break;
+ }
+
+ if (i == vm->def->nhostdevs)
+ goto cleanup;
+
+ dev.data.hostdev = hostdev;
+ if (qemuDomainDetachDeviceLive(vm, &dev, driver, true, true) < 0)
+ goto cleanup;
+
+ cleanup:
+ qemuDomainObjEndJob(driver, vm);
+}
+
+
+static void
+qemuProcessEventHandler(void *data, void *opaque)
{
struct qemuProcessEvent *processEvent = data;
virDomainObjPtr vm = processEvent->vm;
@@ -5057,6 +5400,12 @@ static void qemuProcessEventHandler(void *data, void *opaque)
case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
processRdmaGidStatusChangedEvent(vm, processEvent->data);
break;
+ case QEMU_PROCESS_EVENT_USB_REMOVED:
+ processUSBRemovedEvent(driver, vm, processEvent->data);
+ break;
+ case QEMU_PROCESS_EVENT_USB_ADDED:
+ processUSBAddedEvent(driver, vm, processEvent->data);
+ break;
case QEMU_PROCESS_EVENT_LAST:
break;
}
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 9/9/19 8:33 AM, Nikolay Shirokovskiy wrote:
> Now when code handling attaching/detaching usb hostdev is appropriately
> changed use it to handle host usb device udev add/del events.
Assuming we're ok with adding an extra dependency (libudev),
LGTM.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Small detail below:
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
> src/qemu/Makefile.inc.am | 2 +
> src/qemu/qemu_conf.h | 3 +
> src/qemu/qemu_domain.c | 2 +
> src/qemu/qemu_domain.h | 2 +
> src/qemu/qemu_driver.c | 351 ++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 359 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
> index d16b315ebc..8be0dee396 100644
> --- a/src/qemu/Makefile.inc.am
> +++ b/src/qemu/Makefile.inc.am
> @@ -85,6 +85,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
> -I$(srcdir)/conf \
> -I$(srcdir)/secret \
> $(AM_CFLAGS) \
> + $(UDEV_CFLAGS) \
> $(NULL)
> libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS)
> libvirt_driver_qemu_impl_la_LIBADD = \
> @@ -93,6 +94,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
> $(LIBNL_LIBS) \
> $(SELINUX_LIBS) \
> $(LIBXML_LIBS) \
> + $(UDEV_LIBS) \
> $(NULL)
> libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
>
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 0cbddd7a9c..2e50bb0950 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -294,6 +294,9 @@ struct _virQEMUDriver {
>
> /* Immutable pointer, self-locking APIs */
> virHashAtomicPtr migrationErrors;
> +
> + struct udev_monitor *udev_monitor;
> + int udev_watch;
> };
>
> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 657f3ecfe4..4784804d1e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15034,6 +15034,8 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
> case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
> case QEMU_PROCESS_EVENT_BLOCK_JOB:
> case QEMU_PROCESS_EVENT_MONITOR_EOF:
> + case QEMU_PROCESS_EVENT_USB_REMOVED:
> + case QEMU_PROCESS_EVENT_USB_ADDED:
> VIR_FREE(event->data);
> break;
> case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index d097f23342..94aea62693 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -521,6 +521,8 @@ typedef enum {
> QEMU_PROCESS_EVENT_MONITOR_EOF,
> QEMU_PROCESS_EVENT_PR_DISCONNECT,
> QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
> + QEMU_PROCESS_EVENT_USB_REMOVED,
> + QEMU_PROCESS_EVENT_USB_ADDED,
>
> QEMU_PROCESS_EVENT_LAST
> } qemuProcessEventType;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2378a2e7d0..ce41b0a8d9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -34,6 +34,7 @@
> #include <sys/ioctl.h>
> #include <sys/un.h>
> #include <byteswap.h>
> +#include <libudev.h>
>
>
> #include "qemu_driver.h"
> @@ -719,6 +720,254 @@ qemuDomainFindMaxID(virDomainObjPtr vm,
> }
>
>
> +struct qemuUdevUSBRemoveData {
> + unsigned int bus;
> + unsigned int device;
> +};
> +
> +struct qemuUdevUSBAddData {
> + unsigned int vendor;
> + unsigned int product;
> +};
> +
> +struct qemuUdevUSBEventData {
> + union {
> + struct qemuUdevUSBRemoveData remove;
> + struct qemuUdevUSBAddData add;
> + } data;
> + bool found;
> + bool remove;
> +};
> +
> +static int
> +qemuUdevUSBHandleEvent(virDomainObjPtr vm, void *opaque)
> +{
> + struct qemuUdevUSBEventData *data = opaque;
> + struct qemuProcessEvent *event = NULL;
> + size_t i;
> +
> + if (data->found)
> + return 0;
> +
> + virObjectLock(vm);
> +
> + if (!virDomainObjIsActive(vm))
> + goto cleanup;
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i];
> + virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
> +
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> + continue;
> +
> + if (data->remove) {
> + if (usbsrc->bus != data->data.remove.bus ||
> + usbsrc->device != data->data.remove.device)
> + continue;
> + } else {
> + if (usbsrc->vendor != data->data.add.vendor ||
> + usbsrc->product != data->data.add.product)
> + continue;
> + }
> +
> + data->found = true;
> +
> + if (VIR_ALLOC(event) < 0)
> + goto cleanup;
> +
> + if (data->remove) {
> + struct qemuUdevUSBRemoveData *rm_data;
> +
> +
> + if (VIR_ALLOC(rm_data) < 0)
> + goto cleanup;
> +
> + *rm_data = data->data.remove;
> + event->data = rm_data;
> + event->eventType = QEMU_PROCESS_EVENT_USB_REMOVED;
> + } else {
> + struct qemuUdevUSBAddData *add_data;
> +
> + if (VIR_ALLOC(add_data) < 0)
> + goto cleanup;
> +
> + *add_data = data->data.add;
> + event->data = add_data;
> + event->eventType = QEMU_PROCESS_EVENT_USB_ADDED;
> + }
> +
> + event->vm = virObjectRef(vm);
> +
> + if (virThreadPoolSendJob(qemu_driver->workerPool, 0, event) < 0) {
> + virObjectUnref(vm);
> + goto cleanup;
> + }
> +
> + event = NULL;
> +
> + break;
> + }
> +
> + cleanup:
> + virObjectUnlock(vm);
> +
> + qemuProcessEventFree(event);
> +
> + return 0;
> +}
> +
> +
> +static void
> +qemuUdevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> + int fd ATTRIBUTE_UNUSED,
> + int events ATTRIBUTE_UNUSED,
> + void *data ATTRIBUTE_UNUSED)
> +{
> + struct qemuUdevUSBEventData event_data;
> + struct udev_device *dev = NULL;
> + const char *action;
> + const char *devtype;
> + const char *tmp;
> +
> + /* libvirtd daemon do not run event loop before full state drivers
> + * initialization. Also state drivers uninitialized only after
> + * full stop of event loop. In short driver initialization/uninitialization
> + * and handling events occurs in same main loop thread. Thus we
> + * don't need any locking here. */
> +
> + if (!(dev = udev_monitor_receive_device(qemu_driver->udev_monitor))) {
> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
> + if (errno == EAGAIN || errno == EWOULDBLOCK) {
> + VIR_WARNINGS_RESET
> + return;
> + }
> +
> + virReportSystemError(errno, "%s",
> + _("failed to receive device from udev monitor"));
> + return;
> + }
> +
> + devtype = udev_device_get_devtype(dev);
> +
> + if (STRNEQ_NULLABLE(devtype, "usb_device"))
> + goto cleanup;
> +
> + if (!(action = udev_device_get_action(dev))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive action from udev monitor"));
> + goto cleanup;
> + }
> +
> + if (STREQ(action, "remove")) {
> + struct qemuUdevUSBRemoveData *rm_data = &event_data.data.remove;
> +
> + if (!(tmp = udev_device_get_property_value(dev, "BUSNUM"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive busnum from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 10, &rm_data->bus) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert busnum to int"));
> + goto cleanup;
> + }
> +
> + if (!(tmp = udev_device_get_property_value(dev, "DEVNUM"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive devnum from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 10, &rm_data->device) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert devnum to int"));
> + goto cleanup;
> + }
> + event_data.remove = true;
> + } else if (STREQ(action, "add")) {
> + struct qemuUdevUSBAddData *add_data = &event_data.data.add;
> +
> + if (!(tmp = udev_device_get_property_value(dev, "ID_VENDOR_ID"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive vendor from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 16, &add_data->vendor) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert vendor to int"));
> + goto cleanup;
> + }
> +
> + if (!(tmp = udev_device_get_property_value(dev, "ID_MODEL_ID"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive product from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 16, &add_data->product) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert product to int"));
> + goto cleanup;
> + }
> + event_data.remove = false;
> + }
> +
> + event_data.found = false;
> + virDomainObjListForEach(qemu_driver->domains, qemuUdevUSBHandleEvent, &event_data);
This API recently changed. You'll need to add a 'false' boolean value in the
second parameter.
> +
> + cleanup:
> + udev_device_unref(dev);
> +}
> +
> +
> +static int
> +qemuUdevInitialize(void)
> +{
> + struct udev *udev;
> +
> + if (!(udev = udev_new())) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to create udev context"));
> + return -1;
> + }
> +
> + if (!(qemu_driver->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"))) {
> + udev_unref(udev);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("udev_monitor_new_from_netlink returned NULL"));
> + return -1;
> + }
> +
> + udev_monitor_enable_receiving(qemu_driver->udev_monitor);
> +
> + qemu_driver->udev_watch = virEventAddHandle(udev_monitor_get_fd(qemu_driver->udev_monitor),
> + VIR_EVENT_HANDLE_READABLE,
> + qemuUdevEventHandleCallback, NULL, NULL);
> +
> + if (qemu_driver->udev_watch < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +static void
> +qemuUdevCleanup(void)
> +{
> + if (qemu_driver->udev_monitor) {
> + struct udev *udev = udev_monitor_get_udev(qemu_driver->udev_monitor);
> +
> + udev_monitor_unref(qemu_driver->udev_monitor);
> + udev_unref(udev);
> + qemu_driver->udev_monitor = NULL;
> + }
> +
> + if (qemu_driver->udev_watch > 0) {
> + virEventRemoveHandle(qemu_driver->udev_watch);
> + qemu_driver->udev_watch = 0;
> + }
> +}
> +
> +
> /**
> * qemuStateInitialize:
> *
> @@ -1030,6 +1279,9 @@ qemuStateInitialize(bool privileged,
> if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
> goto error;
>
> + if (qemuUdevInitialize() < 0)
> + goto error;
> +
> /* Get all the running persistent or transient configs first */
> if (virDomainObjListLoadAllConfigs(qemu_driver->domains,
> cfg->stateDir,
> @@ -1239,6 +1491,8 @@ qemuStateCleanup(void)
>
> virLockManagerPluginUnref(qemu_driver->lockManager);
>
> + qemuUdevCleanup();
> +
> virMutexDestroy(&qemu_driver->lock);
> VIR_FREE(qemu_driver);
>
> @@ -5011,7 +5265,96 @@ processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
> }
>
>
> -static void qemuProcessEventHandler(void *data, void *opaque)
> +static void
> +processUSBAddedEvent(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + struct qemuUdevUSBAddData *data)
> +{
> + virDomainHostdevDefPtr hostdev;
> + virDomainHostdevSubsysUSBPtr usbsrc;
> + size_t i;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + return;
> +
> + if (!virDomainObjIsActive(vm)) {
> + VIR_DEBUG("Domain is not running");
> + goto cleanup;
> + }
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + hostdev = vm->def->hostdevs[i];
> +
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> + continue;
> +
> + usbsrc = &hostdev->source.subsys.u.usb;
> +
> + if (usbsrc->vendor == data->vendor && usbsrc->product == data->product)
> + break;
> + }
> +
> + if (i == vm->def->nhostdevs)
> + goto cleanup;
> +
> + if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
> + goto cleanup;
> +
> + cleanup:
> + qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
> +static void
> +processUSBRemovedEvent(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + struct qemuUdevUSBRemoveData *data)
> +{
> + size_t i;
> + virDomainHostdevDefPtr hostdev;
> + virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_HOSTDEV };
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + return;
> +
> + if (!virDomainObjIsActive(vm)) {
> + VIR_DEBUG("Domain is not running");
> + goto cleanup;
> + }
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + virDomainHostdevSubsysUSBPtr usbsrc;
> +
> + hostdev = vm->def->hostdevs[i];
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> + continue;
> +
> + usbsrc = &hostdev->source.subsys.u.usb;
> +
> + /* don't mess with devices that don't use stable host addressing
> + * with respect to unplug/plug to host
> + */
> + if (!usbsrc->vendor || !usbsrc->product)
> + continue;
> +
> + if (usbsrc->bus == data->bus && usbsrc->device == data->device)
> + break;
> + }
> +
> + if (i == vm->def->nhostdevs)
> + goto cleanup;
> +
> + dev.data.hostdev = hostdev;
> + if (qemuDomainDetachDeviceLive(vm, &dev, driver, true, true) < 0)
> + goto cleanup;
> +
> + cleanup:
> + qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
> +static void
> +qemuProcessEventHandler(void *data, void *opaque)
> {
> struct qemuProcessEvent *processEvent = data;
> virDomainObjPtr vm = processEvent->vm;
> @@ -5057,6 +5400,12 @@ static void qemuProcessEventHandler(void *data, void *opaque)
> case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
> processRdmaGidStatusChangedEvent(vm, processEvent->data);
> break;
> + case QEMU_PROCESS_EVENT_USB_REMOVED:
> + processUSBRemovedEvent(driver, vm, processEvent->data);
> + break;
> + case QEMU_PROCESS_EVENT_USB_ADDED:
> + processUSBAddedEvent(driver, vm, processEvent->data);
> + break;
> case QEMU_PROCESS_EVENT_LAST:
> break;
> }
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 09, 2019 at 14:33:07 +0300, Nikolay Shirokovskiy wrote:
> Now when code handling attaching/detaching usb hostdev is appropriately
> changed use it to handle host usb device udev add/del events.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
> src/qemu/Makefile.inc.am | 2 +
> src/qemu/qemu_conf.h | 3 +
> src/qemu/qemu_domain.c | 2 +
> src/qemu/qemu_domain.h | 2 +
> src/qemu/qemu_driver.c | 351 ++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 359 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
> index d16b315ebc..8be0dee396 100644
> --- a/src/qemu/Makefile.inc.am
> +++ b/src/qemu/Makefile.inc.am
> @@ -85,6 +85,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
> -I$(srcdir)/conf \
> -I$(srcdir)/secret \
> $(AM_CFLAGS) \
> + $(UDEV_CFLAGS) \
> $(NULL)
> libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS)
> libvirt_driver_qemu_impl_la_LIBADD = \
> @@ -93,6 +94,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
> $(LIBNL_LIBS) \
> $(SELINUX_LIBS) \
> $(LIBXML_LIBS) \
> + $(UDEV_LIBS) \
> $(NULL)
> libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
>
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 0cbddd7a9c..2e50bb0950 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -294,6 +294,9 @@ struct _virQEMUDriver {
>
> /* Immutable pointer, self-locking APIs */
> virHashAtomicPtr migrationErrors;
> +
> + struct udev_monitor *udev_monitor;
> + int udev_watch;
> };
>
> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 657f3ecfe4..4784804d1e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -15034,6 +15034,8 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
> case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
> case QEMU_PROCESS_EVENT_BLOCK_JOB:
> case QEMU_PROCESS_EVENT_MONITOR_EOF:
> + case QEMU_PROCESS_EVENT_USB_REMOVED:
> + case QEMU_PROCESS_EVENT_USB_ADDED:
> VIR_FREE(event->data);
> break;
> case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index d097f23342..94aea62693 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -521,6 +521,8 @@ typedef enum {
> QEMU_PROCESS_EVENT_MONITOR_EOF,
> QEMU_PROCESS_EVENT_PR_DISCONNECT,
> QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
> + QEMU_PROCESS_EVENT_USB_REMOVED,
> + QEMU_PROCESS_EVENT_USB_ADDED,
>
> QEMU_PROCESS_EVENT_LAST
> } qemuProcessEventType;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 2378a2e7d0..ce41b0a8d9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -34,6 +34,7 @@
> #include <sys/ioctl.h>
> #include <sys/un.h>
> #include <byteswap.h>
> +#include <libudev.h>
>
>
> #include "qemu_driver.h"
> @@ -719,6 +720,254 @@ qemuDomainFindMaxID(virDomainObjPtr vm,
> }
>
>
> +struct qemuUdevUSBRemoveData {
> + unsigned int bus;
> + unsigned int device;
> +};
> +
> +struct qemuUdevUSBAddData {
> + unsigned int vendor;
> + unsigned int product;
> +};
> +
> +struct qemuUdevUSBEventData {
> + union {
> + struct qemuUdevUSBRemoveData remove;
> + struct qemuUdevUSBAddData add;
> + } data;
> + bool found;
> + bool remove;
> +};
> +
> +static int
> +qemuUdevUSBHandleEvent(virDomainObjPtr vm, void *opaque)
> +{
> + struct qemuUdevUSBEventData *data = opaque;
> + struct qemuProcessEvent *event = NULL;
> + size_t i;
> +
> + if (data->found)
> + return 0;
> +
> + virObjectLock(vm);
> +
> + if (!virDomainObjIsActive(vm))
> + goto cleanup;
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i];
> + virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
> +
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> + continue;
> +
> + if (data->remove) {
> + if (usbsrc->bus != data->data.remove.bus ||
> + usbsrc->device != data->data.remove.device)
> + continue;
> + } else {
> + if (usbsrc->vendor != data->data.add.vendor ||
> + usbsrc->product != data->data.add.product)
> + continue;
> + }
I don't see any XML change related to this in this series.
IMO it's unacceptable to re-plug ANY device by default and we must
introduce a flag where the admin gives explicit consent for a device to
be re-plugged on the host hotplug event.
You must note that these two instances may be long time apart and thus
the user might not notice that the device is re-grabbed by the VM.
Also due to the nature of USB devices it may be a completely different
device (e.g. a USB Flash drive of the same make/model but with different
data).
Allowing this by default would lead to confusion.
> +
> + data->found = true;
> +
> + if (VIR_ALLOC(event) < 0)
> + goto cleanup;
> +
> + if (data->remove) {
> + struct qemuUdevUSBRemoveData *rm_data;
> +
> +
> + if (VIR_ALLOC(rm_data) < 0)
> + goto cleanup;
> +
> + *rm_data = data->data.remove;
> + event->data = rm_data;
> + event->eventType = QEMU_PROCESS_EVENT_USB_REMOVED;
> + } else {
> + struct qemuUdevUSBAddData *add_data;
> +
> + if (VIR_ALLOC(add_data) < 0)
> + goto cleanup;
> +
> + *add_data = data->data.add;
> + event->data = add_data;
> + event->eventType = QEMU_PROCESS_EVENT_USB_ADDED;
> + }
> +
> + event->vm = virObjectRef(vm);
> +
> + if (virThreadPoolSendJob(qemu_driver->workerPool, 0, event) < 0) {
> + virObjectUnref(vm);
> + goto cleanup;
> + }
> +
> + event = NULL;
> +
> + break;
> + }
> +
> + cleanup:
> + virObjectUnlock(vm);
> +
> + qemuProcessEventFree(event);
> +
> + return 0;
> +}
> +
> +
> +static void
> +qemuUdevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
> + int fd ATTRIBUTE_UNUSED,
> + int events ATTRIBUTE_UNUSED,
> + void *data ATTRIBUTE_UNUSED)
> +{
> + struct qemuUdevUSBEventData event_data;
> + struct udev_device *dev = NULL;
> + const char *action;
> + const char *devtype;
> + const char *tmp;
> +
> + /* libvirtd daemon do not run event loop before full state drivers
> + * initialization. Also state drivers uninitialized only after
> + * full stop of event loop. In short driver initialization/uninitialization
> + * and handling events occurs in same main loop thread. Thus we
> + * don't need any locking here. */
> +
> + if (!(dev = udev_monitor_receive_device(qemu_driver->udev_monitor))) {
> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
> + if (errno == EAGAIN || errno == EWOULDBLOCK) {
> + VIR_WARNINGS_RESET
> + return;
> + }
> +
> + virReportSystemError(errno, "%s",
> + _("failed to receive device from udev monitor"));
> + return;
> + }
> +
> + devtype = udev_device_get_devtype(dev);
> +
> + if (STRNEQ_NULLABLE(devtype, "usb_device"))
> + goto cleanup;
> +
> + if (!(action = udev_device_get_action(dev))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive action from udev monitor"));
> + goto cleanup;
> + }
> +
> + if (STREQ(action, "remove")) {
> + struct qemuUdevUSBRemoveData *rm_data = &event_data.data.remove;
> +
> + if (!(tmp = udev_device_get_property_value(dev, "BUSNUM"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive busnum from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 10, &rm_data->bus) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert busnum to int"));
> + goto cleanup;
> + }
> +
> + if (!(tmp = udev_device_get_property_value(dev, "DEVNUM"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive devnum from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 10, &rm_data->device) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert devnum to int"));
> + goto cleanup;
> + }
> + event_data.remove = true;
> + } else if (STREQ(action, "add")) {
> + struct qemuUdevUSBAddData *add_data = &event_data.data.add;
> +
> + if (!(tmp = udev_device_get_property_value(dev, "ID_VENDOR_ID"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive vendor from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 16, &add_data->vendor) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert vendor to int"));
> + goto cleanup;
> + }
> +
> + if (!(tmp = udev_device_get_property_value(dev, "ID_MODEL_ID"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to receive product from udev monitor"));
> + goto cleanup;
> + }
> + if (virStrToLong_ui(tmp, NULL, 16, &add_data->product) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Failed to convert product to int"));
> + goto cleanup;
> + }
> + event_data.remove = false;
> + }
> +
> + event_data.found = false;
> + virDomainObjListForEach(qemu_driver->domains, qemuUdevUSBHandleEvent, &event_data);
Is this executed from the event loop thread? If yes we can't do this as
qemuUdevUSBHandleEvent is taking domain locks and thus potentially
waiting indefinitely for any stuck VM.
> +
> + cleanup:
> + udev_device_unref(dev);
> +}
> +
> +
> +static int
> +qemuUdevInitialize(void)
> +{
> + struct udev *udev;
> +
> + if (!(udev = udev_new())) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to create udev context"));
> + return -1;
> + }
> +
> + if (!(qemu_driver->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"))) {
> + udev_unref(udev);
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("udev_monitor_new_from_netlink returned NULL"));
> + return -1;
> + }
> +
> + udev_monitor_enable_receiving(qemu_driver->udev_monitor);
> +
> + qemu_driver->udev_watch = virEventAddHandle(udev_monitor_get_fd(qemu_driver->udev_monitor),
> + VIR_EVENT_HANDLE_READABLE,
> + qemuUdevEventHandleCallback, NULL, NULL);
> +
> + if (qemu_driver->udev_watch < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +
> +static void
> +qemuUdevCleanup(void)
> +{
> + if (qemu_driver->udev_monitor) {
> + struct udev *udev = udev_monitor_get_udev(qemu_driver->udev_monitor);
> +
> + udev_monitor_unref(qemu_driver->udev_monitor);
> + udev_unref(udev);
> + qemu_driver->udev_monitor = NULL;
> + }
> +
> + if (qemu_driver->udev_watch > 0) {
> + virEventRemoveHandle(qemu_driver->udev_watch);
> + qemu_driver->udev_watch = 0;
> + }
> +}
> +
> +
> /**
> * qemuStateInitialize:
> *
> @@ -1030,6 +1279,9 @@ qemuStateInitialize(bool privileged,
> if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
> goto error;
>
> + if (qemuUdevInitialize() < 0)
> + goto error;
> +
> /* Get all the running persistent or transient configs first */
> if (virDomainObjListLoadAllConfigs(qemu_driver->domains,
> cfg->stateDir,
> @@ -1239,6 +1491,8 @@ qemuStateCleanup(void)
>
> virLockManagerPluginUnref(qemu_driver->lockManager);
>
> + qemuUdevCleanup();
> +
> virMutexDestroy(&qemu_driver->lock);
> VIR_FREE(qemu_driver);
>
> @@ -5011,7 +5265,96 @@ processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
> }
>
>
> -static void qemuProcessEventHandler(void *data, void *opaque)
> +static void
> +processUSBAddedEvent(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + struct qemuUdevUSBAddData *data)
> +{
> + virDomainHostdevDefPtr hostdev;
> + virDomainHostdevSubsysUSBPtr usbsrc;
> + size_t i;
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + return;
> +
> + if (!virDomainObjIsActive(vm)) {
> + VIR_DEBUG("Domain is not running");
> + goto cleanup;
> + }
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + hostdev = vm->def->hostdevs[i];
> +
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> + continue;
> +
> + usbsrc = &hostdev->source.subsys.u.usb;
> +
> + if (usbsrc->vendor == data->vendor && usbsrc->product == data->product)
> + break;
> + }
> +
> + if (i == vm->def->nhostdevs)
> + goto cleanup;
> +
> + if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
> + goto cleanup;
> +
> + cleanup:
> + qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
> +static void
> +processUSBRemovedEvent(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + struct qemuUdevUSBRemoveData *data)
> +{
> + size_t i;
> + virDomainHostdevDefPtr hostdev;
> + virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_HOSTDEV };
> +
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + return;
> +
> + if (!virDomainObjIsActive(vm)) {
> + VIR_DEBUG("Domain is not running");
> + goto cleanup;
> + }
> +
> + for (i = 0; i < vm->def->nhostdevs; i++) {
> + virDomainHostdevSubsysUSBPtr usbsrc;
> +
> + hostdev = vm->def->hostdevs[i];
> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
> + continue;
> +
> + usbsrc = &hostdev->source.subsys.u.usb;
> +
> + /* don't mess with devices that don't use stable host addressing
> + * with respect to unplug/plug to host
> + */
> + if (!usbsrc->vendor || !usbsrc->product)
> + continue;
> +
> + if (usbsrc->bus == data->bus && usbsrc->device == data->device)
> + break;
> + }
> +
> + if (i == vm->def->nhostdevs)
> + goto cleanup;
> +
> + dev.data.hostdev = hostdev;
> + if (qemuDomainDetachDeviceLive(vm, &dev, driver, true, true) < 0)
AFAIK this will remove the definition form the XML so how is the re-plug
going to be facilitated then?
> + goto cleanup;
> +
> + cleanup:
> + qemuDomainObjEndJob(driver, vm);
> +}
> +
> +
> +static void
> +qemuProcessEventHandler(void *data, void *opaque)
> {
> struct qemuProcessEvent *processEvent = data;
> virDomainObjPtr vm = processEvent->vm;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 16.09.2019 11:26, Peter Krempa wrote:
> On Mon, Sep 09, 2019 at 14:33:07 +0300, Nikolay Shirokovskiy wrote:
>> Now when code handling attaching/detaching usb hostdev is appropriately
>> changed use it to handle host usb device udev add/del events.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>> src/qemu/Makefile.inc.am | 2 +
>> src/qemu/qemu_conf.h | 3 +
>> src/qemu/qemu_domain.c | 2 +
>> src/qemu/qemu_domain.h | 2 +
>> src/qemu/qemu_driver.c | 351 ++++++++++++++++++++++++++++++++++++++-
>> 5 files changed, 359 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/Makefile.inc.am b/src/qemu/Makefile.inc.am
>> index d16b315ebc..8be0dee396 100644
>> --- a/src/qemu/Makefile.inc.am
>> +++ b/src/qemu/Makefile.inc.am
>> @@ -85,6 +85,7 @@ libvirt_driver_qemu_impl_la_CFLAGS = \
>> -I$(srcdir)/conf \
>> -I$(srcdir)/secret \
>> $(AM_CFLAGS) \
>> + $(UDEV_CFLAGS) \
>> $(NULL)
>> libvirt_driver_qemu_impl_la_LDFLAGS = $(AM_LDFLAGS)
>> libvirt_driver_qemu_impl_la_LIBADD = \
>> @@ -93,6 +94,7 @@ libvirt_driver_qemu_impl_la_LIBADD = \
>> $(LIBNL_LIBS) \
>> $(SELINUX_LIBS) \
>> $(LIBXML_LIBS) \
>> + $(UDEV_LIBS) \
>> $(NULL)
>> libvirt_driver_qemu_impl_la_SOURCES = $(QEMU_DRIVER_SOURCES)
>>
>> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
>> index 0cbddd7a9c..2e50bb0950 100644
>> --- a/src/qemu/qemu_conf.h
>> +++ b/src/qemu/qemu_conf.h
>> @@ -294,6 +294,9 @@ struct _virQEMUDriver {
>>
>> /* Immutable pointer, self-locking APIs */
>> virHashAtomicPtr migrationErrors;
>> +
>> + struct udev_monitor *udev_monitor;
>> + int udev_watch;
>> };
>>
>> virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged);
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 657f3ecfe4..4784804d1e 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -15034,6 +15034,8 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
>> case QEMU_PROCESS_EVENT_SERIAL_CHANGED:
>> case QEMU_PROCESS_EVENT_BLOCK_JOB:
>> case QEMU_PROCESS_EVENT_MONITOR_EOF:
>> + case QEMU_PROCESS_EVENT_USB_REMOVED:
>> + case QEMU_PROCESS_EVENT_USB_ADDED:
>> VIR_FREE(event->data);
>> break;
>> case QEMU_PROCESS_EVENT_JOB_STATUS_CHANGE:
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index d097f23342..94aea62693 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -521,6 +521,8 @@ typedef enum {
>> QEMU_PROCESS_EVENT_MONITOR_EOF,
>> QEMU_PROCESS_EVENT_PR_DISCONNECT,
>> QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
>> + QEMU_PROCESS_EVENT_USB_REMOVED,
>> + QEMU_PROCESS_EVENT_USB_ADDED,
>>
>> QEMU_PROCESS_EVENT_LAST
>> } qemuProcessEventType;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 2378a2e7d0..ce41b0a8d9 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -34,6 +34,7 @@
>> #include <sys/ioctl.h>
>> #include <sys/un.h>
>> #include <byteswap.h>
>> +#include <libudev.h>
>>
>>
>> #include "qemu_driver.h"
>> @@ -719,6 +720,254 @@ qemuDomainFindMaxID(virDomainObjPtr vm,
>> }
>>
>>
>> +struct qemuUdevUSBRemoveData {
>> + unsigned int bus;
>> + unsigned int device;
>> +};
>> +
>> +struct qemuUdevUSBAddData {
>> + unsigned int vendor;
>> + unsigned int product;
>> +};
>> +
>> +struct qemuUdevUSBEventData {
>> + union {
>> + struct qemuUdevUSBRemoveData remove;
>> + struct qemuUdevUSBAddData add;
>> + } data;
>> + bool found;
>> + bool remove;
>> +};
>> +
>> +static int
>> +qemuUdevUSBHandleEvent(virDomainObjPtr vm, void *opaque)
>> +{
>> + struct qemuUdevUSBEventData *data = opaque;
>> + struct qemuProcessEvent *event = NULL;
>> + size_t i;
>> +
>> + if (data->found)
>> + return 0;
>> +
>> + virObjectLock(vm);
>> +
>> + if (!virDomainObjIsActive(vm))
>> + goto cleanup;
>> +
>> + for (i = 0; i < vm->def->nhostdevs; i++) {
>> + virDomainHostdevDefPtr hostdev = vm->def->hostdevs[i];
>> + virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb;
>> +
>> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
>> + continue;
>> +
>> + if (data->remove) {
>> + if (usbsrc->bus != data->data.remove.bus ||
>> + usbsrc->device != data->data.remove.device)
>> + continue;
>> + } else {
>> + if (usbsrc->vendor != data->data.add.vendor ||
>> + usbsrc->product != data->data.add.product)
>> + continue;
>> + }
>
> I don't see any XML change related to this in this series.
>
> IMO it's unacceptable to re-plug ANY device by default and we must
> introduce a flag where the admin gives explicit consent for a device to
> be re-plugged on the host hotplug event.
>
> You must note that these two instances may be long time apart and thus
> the user might not notice that the device is re-grabbed by the VM.
>
> Also due to the nature of USB devices it may be a completely different
> device (e.g. a USB Flash drive of the same make/model but with different
> data).
>
> Allowing this by default would lead to confusion.
Fair enough.
>
>
>> +
>> + data->found = true;
>> +
>> + if (VIR_ALLOC(event) < 0)
>> + goto cleanup;
>> +
>> + if (data->remove) {
>> + struct qemuUdevUSBRemoveData *rm_data;
>> +
>> +
>> + if (VIR_ALLOC(rm_data) < 0)
>> + goto cleanup;
>> +
>> + *rm_data = data->data.remove;
>> + event->data = rm_data;
>> + event->eventType = QEMU_PROCESS_EVENT_USB_REMOVED;
>> + } else {
>> + struct qemuUdevUSBAddData *add_data;
>> +
>> + if (VIR_ALLOC(add_data) < 0)
>> + goto cleanup;
>> +
>> + *add_data = data->data.add;
>> + event->data = add_data;
>> + event->eventType = QEMU_PROCESS_EVENT_USB_ADDED;
>> + }
>> +
>> + event->vm = virObjectRef(vm);
>> +
>> + if (virThreadPoolSendJob(qemu_driver->workerPool, 0, event) < 0) {
>> + virObjectUnref(vm);
>> + goto cleanup;
>> + }
>> +
>> + event = NULL;
>> +
>> + break;
>> + }
>> +
>> + cleanup:
>> + virObjectUnlock(vm);
>> +
>> + qemuProcessEventFree(event);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static void
>> +qemuUdevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
>> + int fd ATTRIBUTE_UNUSED,
>> + int events ATTRIBUTE_UNUSED,
>> + void *data ATTRIBUTE_UNUSED)
>> +{
>> + struct qemuUdevUSBEventData event_data;
>> + struct udev_device *dev = NULL;
>> + const char *action;
>> + const char *devtype;
>> + const char *tmp;
>> +
>> + /* libvirtd daemon do not run event loop before full state drivers
>> + * initialization. Also state drivers uninitialized only after
>> + * full stop of event loop. In short driver initialization/uninitialization
>> + * and handling events occurs in same main loop thread. Thus we
>> + * don't need any locking here. */
>> +
>> + if (!(dev = udev_monitor_receive_device(qemu_driver->udev_monitor))) {
>> + VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
>> + if (errno == EAGAIN || errno == EWOULDBLOCK) {
>> + VIR_WARNINGS_RESET
>> + return;
>> + }
>> +
>> + virReportSystemError(errno, "%s",
>> + _("failed to receive device from udev monitor"));
>> + return;
>> + }
>> +
>> + devtype = udev_device_get_devtype(dev);
>> +
>> + if (STRNEQ_NULLABLE(devtype, "usb_device"))
>> + goto cleanup;
>> +
>> + if (!(action = udev_device_get_action(dev))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to receive action from udev monitor"));
>> + goto cleanup;
>> + }
>> +
>> + if (STREQ(action, "remove")) {
>> + struct qemuUdevUSBRemoveData *rm_data = &event_data.data.remove;
>> +
>> + if (!(tmp = udev_device_get_property_value(dev, "BUSNUM"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to receive busnum from udev monitor"));
>> + goto cleanup;
>> + }
>> + if (virStrToLong_ui(tmp, NULL, 10, &rm_data->bus) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Failed to convert busnum to int"));
>> + goto cleanup;
>> + }
>> +
>> + if (!(tmp = udev_device_get_property_value(dev, "DEVNUM"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to receive devnum from udev monitor"));
>> + goto cleanup;
>> + }
>> + if (virStrToLong_ui(tmp, NULL, 10, &rm_data->device) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Failed to convert devnum to int"));
>> + goto cleanup;
>> + }
>> + event_data.remove = true;
>> + } else if (STREQ(action, "add")) {
>> + struct qemuUdevUSBAddData *add_data = &event_data.data.add;
>> +
>> + if (!(tmp = udev_device_get_property_value(dev, "ID_VENDOR_ID"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to receive vendor from udev monitor"));
>> + goto cleanup;
>> + }
>> + if (virStrToLong_ui(tmp, NULL, 16, &add_data->vendor) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Failed to convert vendor to int"));
>> + goto cleanup;
>> + }
>> +
>> + if (!(tmp = udev_device_get_property_value(dev, "ID_MODEL_ID"))) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to receive product from udev monitor"));
>> + goto cleanup;
>> + }
>> + if (virStrToLong_ui(tmp, NULL, 16, &add_data->product) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Failed to convert product to int"));
>> + goto cleanup;
>> + }
>> + event_data.remove = false;
>> + }
>> +
>> + event_data.found = false;
>> + virDomainObjListForEach(qemu_driver->domains, qemuUdevUSBHandleEvent, &event_data);
>
> Is this executed from the event loop thread? If yes we can't do this as
> qemuUdevUSBHandleEvent is taking domain locks and thus potentially
> waiting indefinitely for any stuck VM.
Yes, this is executed in the event loop thread. But I guess we can take
domain lock here as this lock is intended to be grabbed only for short
periods of time by design (as stated in THREADS.txt). There are a lot
of qemu event handlers that grab domain lock (qemuProcessHandleMonitorEOF
for example). AFAIK we use offloading to thread pool if we need to
start a job which can take long time because of already running job.
And this is offloading is done in qemuUdevUSBHandleEvent.
>
>
>> +
>> + cleanup:
>> + udev_device_unref(dev);
>> +}
>> +
>> +
>> +static int
>> +qemuUdevInitialize(void)
>> +{
>> + struct udev *udev;
>> +
>> + if (!(udev = udev_new())) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("failed to create udev context"));
>> + return -1;
>> + }
>> +
>> + if (!(qemu_driver->udev_monitor = udev_monitor_new_from_netlink(udev, "udev"))) {
>> + udev_unref(udev);
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("udev_monitor_new_from_netlink returned NULL"));
>> + return -1;
>> + }
>> +
>> + udev_monitor_enable_receiving(qemu_driver->udev_monitor);
>> +
>> + qemu_driver->udev_watch = virEventAddHandle(udev_monitor_get_fd(qemu_driver->udev_monitor),
>> + VIR_EVENT_HANDLE_READABLE,
>> + qemuUdevEventHandleCallback, NULL, NULL);
>> +
>> + if (qemu_driver->udev_watch < 0)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static void
>> +qemuUdevCleanup(void)
>> +{
>> + if (qemu_driver->udev_monitor) {
>> + struct udev *udev = udev_monitor_get_udev(qemu_driver->udev_monitor);
>> +
>> + udev_monitor_unref(qemu_driver->udev_monitor);
>> + udev_unref(udev);
>> + qemu_driver->udev_monitor = NULL;
>> + }
>> +
>> + if (qemu_driver->udev_watch > 0) {
>> + virEventRemoveHandle(qemu_driver->udev_watch);
>> + qemu_driver->udev_watch = 0;
>> + }
>> +}
>> +
>> +
>> /**
>> * qemuStateInitialize:
>> *
>> @@ -1030,6 +1279,9 @@ qemuStateInitialize(bool privileged,
>> if (!(qemu_driver->closeCallbacks = virCloseCallbacksNew()))
>> goto error;
>>
>> + if (qemuUdevInitialize() < 0)
>> + goto error;
>> +
>> /* Get all the running persistent or transient configs first */
>> if (virDomainObjListLoadAllConfigs(qemu_driver->domains,
>> cfg->stateDir,
>> @@ -1239,6 +1491,8 @@ qemuStateCleanup(void)
>>
>> virLockManagerPluginUnref(qemu_driver->lockManager);
>>
>> + qemuUdevCleanup();
>> +
>> virMutexDestroy(&qemu_driver->lock);
>> VIR_FREE(qemu_driver);
>>
>> @@ -5011,7 +5265,96 @@ processRdmaGidStatusChangedEvent(virDomainObjPtr vm,
>> }
>>
>>
>> -static void qemuProcessEventHandler(void *data, void *opaque)
>> +static void
>> +processUSBAddedEvent(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + struct qemuUdevUSBAddData *data)
>> +{
>> + virDomainHostdevDefPtr hostdev;
>> + virDomainHostdevSubsysUSBPtr usbsrc;
>> + size_t i;
>> +
>> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> + return;
>> +
>> + if (!virDomainObjIsActive(vm)) {
>> + VIR_DEBUG("Domain is not running");
>> + goto cleanup;
>> + }
>> +
>> + for (i = 0; i < vm->def->nhostdevs; i++) {
>> + hostdev = vm->def->hostdevs[i];
>> +
>> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
>> + continue;
>> +
>> + usbsrc = &hostdev->source.subsys.u.usb;
>> +
>> + if (usbsrc->vendor == data->vendor && usbsrc->product == data->product)
>> + break;
>> + }
>> +
>> + if (i == vm->def->nhostdevs)
>> + goto cleanup;
>> +
>> + if (qemuDomainAttachHostDevice(driver, vm, hostdev) < 0)
>> + goto cleanup;
>> +
>> + cleanup:
>> + qemuDomainObjEndJob(driver, vm);
>> +}
>> +
>> +
>> +static void
>> +processUSBRemovedEvent(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> + struct qemuUdevUSBRemoveData *data)
>> +{
>> + size_t i;
>> + virDomainHostdevDefPtr hostdev;
>> + virDomainDeviceDef dev = { .type = VIR_DOMAIN_DEVICE_HOSTDEV };
>> +
>> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>> + return;
>> +
>> + if (!virDomainObjIsActive(vm)) {
>> + VIR_DEBUG("Domain is not running");
>> + goto cleanup;
>> + }
>> +
>> + for (i = 0; i < vm->def->nhostdevs; i++) {
>> + virDomainHostdevSubsysUSBPtr usbsrc;
>> +
>> + hostdev = vm->def->hostdevs[i];
>> + if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
>> + continue;
>> +
>> + usbsrc = &hostdev->source.subsys.u.usb;
>> +
>> + /* don't mess with devices that don't use stable host addressing
>> + * with respect to unplug/plug to host
>> + */
>> + if (!usbsrc->vendor || !usbsrc->product)
>> + continue;
>> +
>> + if (usbsrc->bus == data->bus && usbsrc->device == data->device)
>> + break;
>> + }
>> +
>> + if (i == vm->def->nhostdevs)
>> + goto cleanup;
>> +
>> + dev.data.hostdev = hostdev;
>> + if (qemuDomainDetachDeviceLive(vm, &dev, driver, true, true) < 0)
>
> AFAIK this will remove the definition form the XML so how is the re-plug
> going to be facilitated then?
"[PATCH v2 02/11] qemu: support host usb device unplug" changes removing
logic to keep unplugged device in the libvirt config.
Nikolay
>
>
>> + goto cleanup;
>> +
>> + cleanup:
>> + qemuDomainObjEndJob(driver, vm);
>> +}
>> +
>> +
>> +static void
>> +qemuProcessEventHandler(void *data, void *opaque)
>> {
>> struct qemuProcessEvent *processEvent = data;
>> virDomainObjPtr vm = processEvent->vm;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Sep 16, 2019 at 08:53:10 +0000, Nikolay Shirokovskiy wrote: > > > On 16.09.2019 11:26, Peter Krempa wrote: > > On Mon, Sep 09, 2019 at 14:33:07 +0300, Nikolay Shirokovskiy wrote: > >> Now when code handling attaching/detaching usb hostdev is appropriately > >> changed use it to handle host usb device udev add/del events. > >> > >> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> > >> --- > >> src/qemu/Makefile.inc.am | 2 + > >> src/qemu/qemu_conf.h | 3 + > >> src/qemu/qemu_domain.c | 2 + > >> src/qemu/qemu_domain.h | 2 + > >> src/qemu/qemu_driver.c | 351 ++++++++++++++++++++++++++++++++++++++- > >> 5 files changed, 359 insertions(+), 1 deletion(-) [...] > > > > Is this executed from the event loop thread? If yes we can't do this as > > qemuUdevUSBHandleEvent is taking domain locks and thus potentially > > waiting indefinitely for any stuck VM. > > Yes, this is executed in the event loop thread. But I guess we can take > domain lock here as this lock is intended to be grabbed only for short > periods of time by design (as stated in THREADS.txt). There are a lot > of qemu event handlers that grab domain lock (qemuProcessHandleMonitorEOF > for example). AFAIK we use offloading to thread pool if we need to > start a job which can take long time because of already running job. > And this is offloading is done in qemuUdevUSBHandleEvent. The problem isn't the time the lock will be held but the unbounded time the VM lock may be held by one of the worker-pool threads executing an API. This would make the event loop stuck until the API finishes. In some cases this same problem exists in our qemu monitor event handler code, where we want to do a per-VM event loop to prevent this issue thus I don't really want to see another instance of this being added. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 16.09.2019 12:03, Peter Krempa wrote: > On Mon, Sep 16, 2019 at 08:53:10 +0000, Nikolay Shirokovskiy wrote: >> >> >> On 16.09.2019 11:26, Peter Krempa wrote: >>> On Mon, Sep 09, 2019 at 14:33:07 +0300, Nikolay Shirokovskiy wrote: >>>> Now when code handling attaching/detaching usb hostdev is appropriately >>>> changed use it to handle host usb device udev add/del events. >>>> >>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> >>>> --- >>>> src/qemu/Makefile.inc.am | 2 + >>>> src/qemu/qemu_conf.h | 3 + >>>> src/qemu/qemu_domain.c | 2 + >>>> src/qemu/qemu_domain.h | 2 + >>>> src/qemu/qemu_driver.c | 351 ++++++++++++++++++++++++++++++++++++++- >>>> 5 files changed, 359 insertions(+), 1 deletion(-) > > [...] > >>> >>> Is this executed from the event loop thread? If yes we can't do this as >>> qemuUdevUSBHandleEvent is taking domain locks and thus potentially >>> waiting indefinitely for any stuck VM. >> >> Yes, this is executed in the event loop thread. But I guess we can take >> domain lock here as this lock is intended to be grabbed only for short >> periods of time by design (as stated in THREADS.txt). There are a lot >> of qemu event handlers that grab domain lock (qemuProcessHandleMonitorEOF >> for example). AFAIK we use offloading to thread pool if we need to >> start a job which can take long time because of already running job. >> And this is offloading is done in qemuUdevUSBHandleEvent. > > The problem isn't the time the lock will be held but the unbounded time > the VM lock may be held by one of the worker-pool threads executing an > API. > > This would make the event loop stuck until the API finishes. API implementation releases domain lock before running long operation like executing qemu command or executing migration phase on destination. > > In some cases this same problem exists in our qemu monitor event handler > code, where we want to do a per-VM event loop to prevent this issue thus > I don't really want to see another instance of this being added. > Ok. Looks like it does not make sense to grab VM lock in event loop in this particular case. AFAIU grabbing VM lock makes sense when we need to notify some API thread waiting for event. Nikolay -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.