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;