[virt-tools-list] [PATCH v2] domain: add 'pre-startup' signal and do nodedevs checking

[Cole Robinson]

On 05/11/2013 11:33 AM, Cole Robinson wrote:
> On 05/10/2013 05:49 AM, Guannan Ren wrote:
>> This patch introduces 'pre-start' signal and registers
>> nodedev checking handler to check duplicate USB devices.
>> If virt-manager can not identify unique usb device any more
>> before domain startup, it will throw a tip error to tell it
>> is time to reattach host USB devices to get updated bus/addr info.
>> ---
>>  virtManager/domain.py | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>
>> diff --git a/virtManager/domain.py b/virtManager/domain.py
>> index 791f2af..26cff37 100644
>> --- a/virtManager/domain.py
>> +++ b/virtManager/domain.py
>> @@ -149,6 +149,7 @@ class vmmDomain(vmmLibvirtObject):
>>          "status-changed": (GObject.SignalFlags.RUN_FIRST, None, [int, int]),
>>          "resources-sampled": (GObject.SignalFlags.RUN_FIRST, None, []),
>>          "inspection-changed": (GObject.SignalFlags.RUN_FIRST, None, []),
>> +        "pre-startup": (GObject.SignalFlags.RUN_FIRST, None, [])
>>      }
>>  
>>      def __init__(self, conn, backend, uuid):
>> @@ -194,6 +195,9 @@ class vmmDomain(vmmLibvirtObject):
>>          self._stats_disk_supported = True
>>          self._stats_disk_skip = []
>>  
>> +        # pre_startup global flags
>> +        self._prestartup_hostdev_check_failed = False
>> +
>>          self.inspection = vmmInspectionData()
>>  
>>          if isinstance(self._backend, virtinst.Guest):
>> @@ -252,7 +256,59 @@ class vmmDomain(vmmLibvirtObject):
>>  
>>          self.connect("status-changed", self._update_start_vcpus)
>>          self.connect("config-changed", self._reparse_xml)
>> +        self.connect("pre-startup", self._prestartup_nodedev_check)
>> +
>> +    #####################
>> +    # checking routines #
>> +    #####################
>> +
>> +    def attrVal(self, attr):
>> +        if hasattr(self, attr):
>> +            return getattr(self, attr)
>> +        return False
>> +
>> +    # class closure to initialize global variable.
>> +    def do_pre_startup(self):
>> +        if getattr(self, "_prestartup_hostdev_check_failed"):
>> +            self._prestartup_hostdev_check_failed = False
>> +            # Initialize falure reasons
>> +            self._hostdev_non_exist = False
>> +            self._hostdev_multiples = False
>>  
>> +    def _prestartup_nodedev_check(self, data=None):
>> +        for hostdev in self.get_hostdev_devices():
>> +            devtype = hostdev.type
>> +
>> +            if devtype != "usb":
>> +                continue
>> +
>> +            vendor = hostdev.vendor
>> +            product = hostdev.product
>> +            bus = hostdev.bus
>> +            device = hostdev.device
>> +
>> +            if vendor and product:
>> +                count = self.conn.get_nodedevs_number("usb_device",
>> +                                                      vendor,
>> +                                                      product)
>> +                if not count:
>> +                    self._prestartup_hostdev_check_failed = True
>> +                    self._hostdev_non_exist = True
>> +
>> +                elif count > 1 and not (bus and device):
>> +                    self._prestartup_hostdev_check_failed = True
>> +                    self._hostdev_multiples = True
>> +
>> +    def _prestartup_nodedev_report(self):
>> +        if self._prestartup_hostdev_check_failed:
>> +            if self.attrVal("_hostdev_non_exist"):
>> +                raise RuntimeError(_("Could not find any USB device"))
>> +
> 
> I would drop this error and just let libvirt/qemu handle it for us.
> 
>> +            elif self.attrVal("_hostdev_multiples"):
>> +                raise RuntimeError(_("The attached USB device "
>> +                                     "is not unique, please remove "
>> +                                     "and add it again!"))
> 
> I'd change this message to:
> 
> There is more than one '%(prettyname)s' device attached to your host, and we
> can't determine which one to use for your guest.
> 
> To fix this, remove and reattach the USB device to your guest using the 'Add
> Hardware' wizard.
> 
>> +        return;
>>  
>>      ###########################
>>      # Misc API getter methods #
>> @@ -1171,6 +1227,10 @@ class vmmDomain(vmmLibvirtObject):
>>          if self.get_cloning():
>>              raise RuntimeError(_("Cannot start guest while cloning "
>>                                   "operation in progress"))
>> +
>> +        self.emit("pre-startup")
>> +        self._prestartup_nodedev_report()
>> +
>>          self._backend.create()
>>          self.idle_add(self.force_update_status)
>>  
>>
> 
> Hmm, that return value mangling is sub-optimal, but indeed I can't get emit to
> return values like I thought it could.
> 
> But I remembered a trick we use elsewhere in virt-manager already. If you pass
> a list as a signal argument, the signal handler can add values to it, which
> the emitter can than access. You can then just have the signal callback return
> the string error directly.
> 
> Here's an example to show how it works:
> 
> 
> diff --git a/virtManager/domain.py b/virtManager/domain.py
> index 7c3f952..f2a9041 100644
> --- a/virtManager/domain.py
> +++ b/virtManager/domain.py
> @@ -149,7 +149,7 @@ class vmmDomain(vmmLibvirtObject):
>          "status-changed": (GObject.SignalFlags.RUN_FIRST, None, [int, int]),
>          "resources-sampled": (GObject.SignalFlags.RUN_FIRST, None, []),
>          "inspection-changed": (GObject.SignalFlags.RUN_FIRST, None, []),
> -        "pre-startup": (GObject.SignalFlags.RUN_FIRST, None, [])
> +        "pre-startup": (GObject.SignalFlags.RUN_FIRST, None, [object])
>      }
> 
>      def __init__(self, conn, backend, uuid):
> @@ -275,7 +275,10 @@ class vmmDomain(vmmLibvirtObject):
>              self._hostdev_non_exist = False
>              self._hostdev_multiples = False
> 
> -    def _prestartup_nodedev_check(self, data=None):
> +    def _prestartup_nodedev_check(self, src, ret):
> +        print "in hook ret", ret
> +        ret.append("frobber")
> +        return
>          for hostdev in self.get_hostdev_devices():
>              devtype = hostdev.type
> 
> @@ -1228,7 +1231,9 @@ class vmmDomain(vmmLibvirtObject):
>              raise RuntimeError(_("Cannot start guest while cloning "
>                                   "operation in progress"))
> 
> -        self.emit("pre-startup")
> +        ret = []
> +        self.emit("pre-startup", ret)
> +        print "emit ret", ret
>          self._prestartup_nodedev_report()
> 
>          self._backend.create()
> 

Also I just pushed a commit to tests/testdriver.xml to add duplicate devices,
and a guest named test-duplicate-usb so you can this logic without having to
interact with real guests/hardware.

- Cole