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

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



On Tue, Jan 12, 2010 at 03:26:29PM -0500, Cole Robinson wrote:
> 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..e015c06 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -484,7 +484,9 @@ SELinuxSetSecurityHostdevLabel(virConnectPtr conn,
>          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);
> +                                          dev->source.subsys.u.usb.device,
> +                                          dev->source.subsys.u.usb.vendor,
> +                                          dev->source.subsys.u.usb.product);
>  
>              if (!usb)
>                  goto done;

You need to remove the surrounding  if/else clause here


> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 35b29ad..6a52d4c 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -839,8 +839,11 @@ get_files(vahControl * ctl)
>                  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);
> +                                          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,

And likewise here.

> diff --git a/src/util/hostusb.c b/src/util/hostusb.c
> index 07e10b1..3e9ac83 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,95 @@ 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,
> +                          const char *fmt, unsigned *value)
> +{
> +    int ret = -1;
> +    char *buf = NULL;
> +    char filename[PATH_MAX];
> +
> +    snprintf(filename, PATH_MAX, USB_SYSFS "/devices/%s/%s", d_name, f_name);

Lets us virAsprintf() here.

> +
> +    if (virFileReadAll(filename, 1024, &buf) < 0)
> +        goto error;
> +
> +    if (sscanf(buf, fmt, value) != 1) {
> +        virReportSystemError(conn, errno,
> +                             _("Could not parse usb file %s"), filename);
> +        goto error;
> +    }

The callers all either pass in "%x" or "%d" to this, so how about just
using virStrToLong  and passing in the 'base' arg instead of format.

> +static int usbFindBusByVendor(virConnectPtr conn,
> +                              unsigned vendor, unsigned product,
> +                              unsigned *bus, unsigned *devno)
> +{
> +    DIR *dir = NULL;
> +    int ret = -1, found = 0;
> +    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,
> +                           "%x", &found_vend) < 0)
> +            goto error;
> +        if (usbSysReadFile(conn, "idProduct", de->d_name,
> +                           "%x", &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;
> +            found_bus = atoi(tmpstr);

This should be virStrToLong

> +
> +            if (usbSysReadFile(conn, "devnum", de->d_name,
> +                               "%d", &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 +155,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);
>  
> -- 


Overall, it is alot simpler than I was expecting it to be which is nice ! 


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]