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@xxxxxxxxxxxxx>
> ---
> 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;
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
