Re: [libvirt] [PATCH v2 04/11] qemu: handle host usb device add/del udev events

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(a)virtuozzo.com&gt; > --- > 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;