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

Re: [libvirt] [PATCH v2] Implement path lookup for USB by vendor:product



On Wed, Jan 13, 2010 at 03:50:05PM -0500, Cole Robinson wrote:
> Based off how QEMU does it, look through /sys/bus/usb/devices/* for
> matching vendor:product info, and if found, use info from the surrounding
> files to build the device's /dev/bus/usb path.
> 
> This fixes USB device assignment by vendor:product when running qemu
> as non-root (well, it should, but for some reason I couldn't reproduce
> the failure people are seeing in [1], but it appears to work properly)
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=542450
> 
> v2:
>     Drop 'bus.addr only' checks in security drivers
>     Use various util helpers
> 
> Signed-off-by: Cole Robinson <crobinso redhat com>
> ---
>  po/POTFILES.in                  |    1 +
>  src/qemu/qemu_driver.c          |    9 +--
>  src/security/security_selinux.c |   25 ++++-----
>  src/security/virt-aa-helper.c   |   32 +++++------
>  src/util/hostusb.c              |  110 +++++++++++++++++++++++++++++++++++++-
>  src/util/hostusb.h              |    4 +-
>  6 files changed, 141 insertions(+), 40 deletions(-)
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index c9c78b6..3b82a74 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -55,6 +55,7 @@ src/uml/uml_conf.c
>  src/uml/uml_driver.c
>  src/util/bridge.c
>  src/util/conf.c
> +src/util/hostusb.c
>  src/util/json.c
>  src/util/logging.c
>  src/util/pci.c
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index deb8adc..f03ce91 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2105,14 +2105,11 @@ static int qemuDomainSetHostdevUSBOwnership(virConnectPtr conn,
>      struct qemuFileOwner owner = { uid, gid };
>      int ret = -1;
>  
> -    /* XXX what todo for USB devs assigned based on product/vendor ? Doom :-( */
> -    if (!def->source.subsys.u.usb.bus ||
> -        !def->source.subsys.u.usb.device)
> -        return 0;
> -
>      usbDevice *dev = usbGetDevice(conn,
>                                    def->source.subsys.u.usb.bus,
> -                                  def->source.subsys.u.usb.device);
> +                                  def->source.subsys.u.usb.device,
> +                                  def->source.subsys.u.usb.vendor,
> +                                  def->source.subsys.u.usb.product);
>  
>      if (!dev)
>          goto cleanup;
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 000bc8a..cb585ed 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -481,20 +481,17 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
>  
>      switch (dev->source.subsys.type) {
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
> -        if (dev->source.subsys.u.usb.bus && dev->source.subsys.u.usb.device) {
> -            usbDevice *usb = usbGetDevice(conn,
> -                                          dev->source.subsys.u.usb.bus,
> -                                          dev->source.subsys.u.usb.device);
> +        usbDevice *usb = usbGetDevice(conn,
> +                                      dev->source.subsys.u.usb.bus,
> +                                      dev->source.subsys.u.usb.device,
> +                                      dev->source.subsys.u.usb.vendor,
> +                                      dev->source.subsys.u.usb.product);
>  
> -            if (!usb)
> -                goto done;
> +        if (!usb)
> +            goto done;
>  
> -            ret = usbDeviceFileIterate(conn, usb, SELinuxSetSecurityUSBLabel, vm);
> -            usbFreeDevice(conn, usb);
> -        } else {
> -            /* XXX deal with product/vendor better */
> -            ret = 0;
> -        }
> +        ret = usbDeviceFileIterate(conn, usb, SELinuxSetSecurityUSBLabel, vm);
> +        usbFreeDevice(conn, usb);
>          break;
>      }
>  
> @@ -556,7 +553,9 @@ SELinuxRestoreSecurityHostdevLabel(virConnectPtr conn,
>      case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
>          usbDevice *usb = usbGetDevice(conn,
>                                        dev->source.subsys.u.usb.bus,
> -                                      dev->source.subsys.u.usb.device);
> +                                      dev->source.subsys.u.usb.device,
> +                                      dev->source.subsys.u.usb.vendor,
> +                                      dev->source.subsys.u.usb.product);
>  
>          if (!usb)
>              goto done;
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 35b29ad..3c8b49a 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -836,24 +836,22 @@ get_files(vahControl * ctl)
>              virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
>              switch (dev->source.subsys.type) {
>              case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
> -                if (dev->source.subsys.u.usb.bus &&
> -                    dev->source.subsys.u.usb.device) {
> -                    usbDevice *usb = usbGetDevice(NULL,
> -                               dev->source.subsys.u.usb.bus,
> -                               dev->source.subsys.u.usb.device);
> -                    if (usb == NULL)
> -                        continue;
> -                    rc = usbDeviceFileIterate(NULL, usb,
> -                                              file_iterate_cb, &buf);
> -                    usbFreeDevice(NULL, usb);
> -                    if (rc != 0)
> -                        goto clean;
> -                    else {
> -                        /* TODO: deal with product/vendor better */
> -                        rc = 0;
> -                    }
> -                }
> +                usbDevice *usb = usbGetDevice(NULL,
> +                                          dev->source.subsys.u.usb.bus,
> +                                          dev->source.subsys.u.usb.device,
> +                                          dev->source.subsys.u.usb.vendor,
> +                                          dev->source.subsys.u.usb.product);
> +
> +                if (usb == NULL)
> +                    continue;
> +
> +                rc = usbDeviceFileIterate(NULL, usb,
> +                                          file_iterate_cb, &buf);
> +                usbFreeDevice(NULL, usb);
> +                if (rc != 0)
> +                    goto clean;
>                  break;
> +                }
>              }
>  /* TODO: update so files in /sys are readonly
>              case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> diff --git a/src/util/hostusb.c b/src/util/hostusb.c
> index 07e10b1..8fbb486 100644
> --- a/src/util/hostusb.c
> +++ b/src/util/hostusb.c
> @@ -37,9 +37,10 @@
>  #include "util.h"
>  #include "virterror_internal.h"
>  
> +#define USB_SYSFS "/sys/bus/usb"
>  #define USB_DEVFS "/dev/bus/usb/"
> -#define USB_ID_LEN 10 /* "XXXX XXXX" */
> -#define USB_ADDR_LEN 8 /* "XXX:XXX" */
> +#define USB_ID_LEN 10 /* "1234 5678" */
> +#define USB_ADDR_LEN 8 /* "123:456" */
>  
>  struct _usbDevice {
>      unsigned      bus;
> @@ -57,11 +58,108 @@ struct _usbDevice {
>      virReportErrorHelper(conn, VIR_FROM_NONE, code, __FILE__,  \
>                           __FUNCTION__, __LINE__, fmt)
>  
> +static int usbSysReadFile(virConnectPtr conn,
> +                          const char *f_name, const char *d_name,
> +                          int base, unsigned *value)
> +{
> +    int ret = -1, tmp;
> +    char *buf = NULL;
> +    char *filename = NULL;
> +    char *ignore = NULL;
> +
> +    tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name, f_name);
> +    if (tmp < 0) {
> +        virReportOOMError(conn);
> +        goto error;
> +    }
> +
> +    if (virFileReadAll(filename, 1024, &buf) < 0)
> +        goto error;
> +
> +    if (virStrToLong_ui(buf, &ignore, base, value) < 0) {
> +        usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                       _("Could not parse usb file %s"), filename);
> +        goto error;
> +    }
> +
> +    ret = 0;
> +error:
> +    VIR_FREE(filename);
> +    VIR_FREE(buf);
> +    return ret;
> +}
> +
> +static int usbFindBusByVendor(virConnectPtr conn,
> +                              unsigned vendor, unsigned product,
> +                              unsigned *bus, unsigned *devno)
> +{
> +    DIR *dir = NULL;
> +    int ret = -1, found = 0;
> +    char *ignore = NULL;
> +    struct dirent *de;
> +
> +    dir = opendir(USB_SYSFS "/devices");
> +    if (!dir) {
> +        virReportSystemError(conn, errno,
> +                             _("Could not open directory %s"),
> +                             USB_SYSFS "/devices");
> +        goto error;
> +    }
> +
> +    while ((de = readdir(dir))) {
> +        unsigned found_prod, found_vend;
> +        if (de->d_name[0] == '.' || strchr(de->d_name, ':'))
> +            continue;
> +
> +        if (usbSysReadFile(conn, "idVendor", de->d_name,
> +                           16, &found_vend) < 0)
> +            goto error;
> +        if (usbSysReadFile(conn, "idProduct", de->d_name,
> +                           16, &found_prod) < 0)
> +            goto error;
> +
> +        if (found_prod == product && found_vend == vendor) {
> +            /* Lookup bus.addr info */
> +            char *tmpstr = de->d_name;
> +            unsigned found_bus, found_addr;
> +
> +            if (STREQ(de->d_name, "usb"))
> +                tmpstr += 3;
> +
> +            if (virStrToLong_ui(tmpstr, &ignore, 10, &found_bus) < 0) {
> +                usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to parse dir name '%s'"),
> +                               de->d_name);
> +                goto error;
> +            }
> +
> +            if (usbSysReadFile(conn, "devnum", de->d_name,
> +                               10, &found_addr) < 0)
> +                goto error;
> +
> +            *bus = found_bus;
> +            *devno = found_addr;
> +            found = 1;
> +            break;
> +        }
> +    }
> +
> +    if (!found)
> +        usbReportError(conn, VIR_ERR_INTERNAL_ERROR,
> +                       _("Did not find USB device %x:%x"), vendor, product);
> +    else
> +        ret = 0;
> +
> +error:
> +    return ret;
> +}
>  
>  usbDevice *
>  usbGetDevice(virConnectPtr conn,
>               unsigned bus,
> -             unsigned devno)
> +             unsigned devno,
> +             unsigned vendor,
> +             unsigned product)
>  {
>      usbDevice *dev;
>  
> @@ -70,6 +168,12 @@ usbGetDevice(virConnectPtr conn,
>          return NULL;
>      }
>  
> +    if (vendor) {
> +        /* Look up bus.dev by vendor:product */
> +        if (usbFindBusByVendor(conn, vendor, product, &bus, &devno) < 0)
> +            return NULL;
> +    }
> +
>      dev->bus     = bus;
>      dev->dev     = devno;
>  
> diff --git a/src/util/hostusb.h b/src/util/hostusb.h
> index 7f75c8b..739a4aa 100644
> --- a/src/util/hostusb.h
> +++ b/src/util/hostusb.h
> @@ -28,7 +28,9 @@ typedef struct _usbDevice usbDevice;
>  
>  usbDevice *usbGetDevice      (virConnectPtr  conn,
>                                unsigned       bus,
> -                              unsigned       devno);
> +                              unsigned       devno,
> +                              unsigned       vendor,
> +                              unsigned       product);
>  void       usbFreeDevice     (virConnectPtr  conn,
>                                usbDevice     *dev);
>  
> -- 

ACK

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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