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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@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.

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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux