[virt-tools-list] [PATCH 2/2] addhardware: use bus, device to distingush usb products

[Cole Robinson]

On 04/22/2013 01:45 PM, Guannan Ren wrote:
> When the usb device being attached is unique, the
> vendorId and productId is being used in XML.
> When there are multiple usb devices with same vendorId
> and productId in the host device list, the bus and device
> number is being used to attach the device for virt-manager.
> ---
>  virtManager/addhardware.py    | 21 +++++++++++++++++++++
>  virtManager/connection.py     | 20 ++++++++++++++++++++
>  virtinst/VirtualHostDevice.py |  5 ++---
>  3 files changed, 43 insertions(+), 3 deletions(-)
> 

Please squash in patch 1, it's not useful on its own.

> diff --git a/virtManager/addhardware.py b/virtManager/addhardware.py
> index 4c1d4ac..572b04c 100644
> --- a/virtManager/addhardware.py
> +++ b/virtManager/addhardware.py
> @@ -1415,6 +1415,27 @@ class vmmAddHardware(vmmGObjectUI):
>          except Exception, e:
>              return self.err.val_err(_("Host device parameter error"), e)
>  
> +        devtype = ret[2]
> +        if devtype == "usb_device":
> +            vendor = self._dev.vendor
> +            product = self._dev.product
> +            count = self.conn.get_nodedev_numbers(devtype, vendor, product)
> +            if count > 1:
> +                self._dev.vendor = None
> +                self._dev.product = None
> +                return
> +
> +            if count == 1:
> +                self._dev.bus = None
> +                self._dev.device = None
> +                return
> +
> +            if not count:
> +                raise RuntimeError(_("Could not find USB device "
> +                                     "(vendorId: %s, productId: %s) "
> +                                     % (vendor, product)))
> +
> +
>      def validate_page_char(self):
>          chartype = self.get_char_type()
>          modebox = self.widget("char-mode")
> diff --git a/virtManager/connection.py b/virtManager/connection.py
> index cd340ac..c8767b7 100644
> --- a/virtManager/connection.py
> +++ b/virtManager/connection.py
> @@ -705,6 +705,26 @@ class vmmConnection(vmmGObject):
>  
>          return retdevs
>  
> +    def get_nodedev_numbers(self, devtype, vendor, product):
> +        count = 0
> +        self._ticklock.acquire()
> +
> +        for dev in self.nodedevs.values():
> +            vdev = dev.get_virtinst_obj()
> +            if devtype and vdev.device_type != devtype:
> +                continue
> +
> +            if vendor == vdev.vendor_id and \
> +                product == vdev.product_id:
> +                count += 1
> +
> +        self._ticklock.release()
> +        logging.debug("There are %d node devices with "
> +                      "vendorId: %s, productId: %s",
> +                       count, vendor, product)
> +
> +        return count
> +

The ticklock bit is dangerous here, if we somehow hit an exception, the lock
won't be released.

But I'd just drop the ticklock bits anyways. Yeah we definitely have some
issues with the tick thread updating bits in a potentially dangerous way, but
unless you were hitting issues in practice, don't bother. We need a
comprehensive solution rather than piecemeal.

Once you do that, just move the function into addhardware since that's the
only user.

(without thinking too much, we should probably do all the Connection state
updating in the idle callback. but not sure how that plays into invoking
vm.tick(). If anyone wanted to tackle it I can give feedback but in practice
it's not a big issue)

>      def get_net_by_name(self, name):
>          for net in self.nets.values():
>              if net.get_name() == name:
> diff --git a/virtinst/VirtualHostDevice.py b/virtinst/VirtualHostDevice.py
> index f5740d0..ee38242 100644
> --- a/virtinst/VirtualHostDevice.py
> +++ b/virtinst/VirtualHostDevice.py
> @@ -215,9 +215,8 @@ class VirtualHostDeviceUSB(VirtualHostDevice):
>          self.vendor = nodedev.vendor_id
>          self.product = nodedev.product_id
>  
> -        if not (self.vendor or self.product):
> -            self.bus = nodedev.bus
> -            self.device = nodedev.device
> +        self.bus = nodedev.bus
> +        self.device = nodedev.device
>  
>      def _get_source_xml(self):
>          xml = ""
> 

I know this was involved in breaking the test suite in the past, though I
think it required another virtinst change. This definitely doesn't cause
python setup.py test regressions right?

Thanks,
Cole