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

2010-01-13 Thread Daniel P. Berrange 
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 /*   */
 -#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

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

2010-01-12 Thread Cole Robinson 
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

Signed-off-by: Cole Robinson crobi...@redhat.com
---
 po/POTFILES.in  |1 +
 src/qemu/qemu_driver.c  |9 +--
 src/security/security_selinux.c |8 ++-
 src/security/virt-aa-helper.c   |7 ++-
 src/util/hostusb.c  |   97 +-
 src/util/hostusb.h  |4 +-
 6 files changed, 112 insertions(+), 14 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 1ab0859..22e9c3c 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -54,6 +54,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..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;
@@ -556,7 +558,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..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,
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 /*   */
-#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__,