[virt-tools-list] [PATCH] Virt-manager host device information bits

[Michal Novotny]

On 11/03/2009 06:53 PM, Cole Robinson wrote:
> On 11/03/2009 12:24 PM, Michal Novotny wrote:
>    
>> Hi,
>> this is new patch for host device information to just find&  show pretty
>> name of device for USB and PCI devices, tested with test driver so
>> please review...
>>
>> Thanks,
>> Michal
>>
>>      
> Comments inline
>
>    
>> diff -r a5b9807ead04 src/virtManager/details.py
>> --- a/src/virtManager/details.py	Sun Nov 01 21:56:21 2009 -0500
>> +++ b/src/virtManager/details.py	Tue Nov 03 18:19:29 2009 +0100
>> @@ -913,12 +913,76 @@
>>           if not hostdevinfo:
>>               return
>>
>> +        try:
>> +            typ = hostdevinfo[1]['type']
>> +            if typ == 'usb':
>> +                typ = 'usb_device'
>> +            vendor_id = hostdevinfo[1]['vendor']['id']
>> +            product_id = hostdevinfo[1]['product']['id']
>> +            device = 0
>> +            bus = 0
>> +            domain = 0
>> +            func = 0
>> +            slot = 0
>> +        except Exception, e:
>> +            try:
>> +                device = int(hostdevinfo[1]['address']['device'] or '0x00', 16)
>> +                bus = int(hostdevinfo[1]['address']['bus'] or '0x00', 16)
>> +                domain = int(hostdevinfo[1]['address']['domain'] or '0x00', 16)
>> +                func = int(hostdevinfo[1]['address']['function'] or '0x00', 16)
>> +                slot = int(hostdevinfo[1]['address']['slot'] or '0x00', 16)
>> +                vendor_id = -1
>> +                product_id = -1
>> +            except Exception, e:
>> +                device = 0
>> +                bus = 0
>> +                domain = 0
>> +                func = 0
>> +                slot = 0
>> +                vendor_id = -1
>> +                product_id = -1
>> +
>>      
> Rather than do all this inside try...except blocks, you can just use
> dict.get():
>
> devinfo = hostdevinfo[1]
> if devinfo.get("vendor") and devinfo.get("product"):
>      vendor_id = devinfo["vendor"].get("id") or -1
>      product_id = devinfo["product"].get("id") or -1
> elif devinfo.get("address"):
>      ...
>
> etc.
>    

Well, thanks for the tip. I'll rewrite it this way tomorrow...

>    
>> +        dev_pretty_name = None
>> +        devs = self.vm.get_connection().get_devices( typ, None )
>> +
>> +        # Try to get info from {product|vendor}_id
>> +        for dev in devs:
>> +            if dev.product_id == product_id and dev.vendor_id == vendor_id:
>> +                dev_pretty_name = dev.pretty_name()
>> +
>>      
> Probably want to 'break' after a successful lookup
>
>    

It may speed up the code a little, that's right...

>> +        if not dev_pretty_name:
>> +            # Try to get info from bus/addr
>> +            for dev in devs:
>> +                try:
>> +                    dev_id = int(dev.device)
>>      
> Rather than compare by 'int', just keep the
> hostdevinfo[1]["address"]["device"] in string form and compare like that.
>
>    

Well, I was running into some issues ... when having the required 
variables, it was clearly int() type... ie. `device` is a required 
variable... but when accessing dev.device, I am getting eg. 0x0000 which 
is hexadecimal so the comparison is not available because one is int() 
and second is hex()...

>> +                except Exception, e:
>> +                    dev_id = -1
>> +                try:
>> +                    bus_id = int(dev.bus)
>> +                except Exception, e:
>> +                    bus_id = -1
>> +                try:
>> +                    domain_id = int(dev.domain)
>> +                except Exception, e:
>> +                    domain_id = -1
>> +                try:
>> +                    func_id = int(dev.function)
>> +                except Exception, e:
>> +                    func_id = -1
>> +                try:
>> +                    slot_id = int(dev.slot)
>> +                except Exception, e:
>> +                    slot_id = -1
>> +
>> +                if ((dev_id == device and bus_id == bus)
>> +                  or (domain_id == domain and func_id == func and slot_id == slot)):
>> +                    dev_pretty_name = dev.pretty_name()
>> +
>>      
> This should all be in the same 'for dev in devs' loop. If the
> product/vendor lookup fails, just do
>
> else if dev_id == device and bus_id == bus ...
>
>    

Ok, I can rewrite this as well...

>> +
>>           devlabel = "<b>Physical %s Device</b>" % hostdevinfo[4].upper()
>>
>>           self.window.get_widget("hostdev-title").set_markup(devlabel)
>> -        self.window.get_widget("hostdev-type").set_text(hostdevinfo[4])
>> -        self.window.get_widget("hostdev-mode").set_text(hostdevinfo[3])
>> -        self.window.get_widget("hostdev-source").set_text(hostdevinfo[5])
>> +        self.window.get_widget("hostdev-source").set_text(dev_pretty_name or "-")
>>
>>       def refresh_video_page(self):
>>           vidinfo = self.get_hw_selection(HW_LIST_COL_DEVICE)
>> diff -r a5b9807ead04 src/vmm-details.glade
>> --- a/src/vmm-details.glade	Sun Nov 01 21:56:21 2009 -0500
>> +++ b/src/vmm-details.glade	Tue Nov 03 18:19:29 2009 +0100
>> @@ -6,7 +6,6 @@
>>       <property name="title" translatable="yes">Virtual Machine</property>
>>       <property name="default_width">800</property>
>>       <property name="default_height">600</property>
>> -<signal name="delete_event" handler="on_vmm_details_delete_event"/>
>>       <child>
>>         <widget class="GtkVBox" id="vbox2">
>>           <property name="visible">True</property>
>>      
> Urgh, broken glade-3 strikes again. Can't remember what version it is,
> but glade likes to drop the delete signal handler for windows, which
> means hitting the 'X' in the top corner destroys the window rather than
> just hiding it. Next time you go to launch the same window, virt-manager
> will error.
>
>    

This is the first time I've been trying to use Glade-3 designer because 
I've always tried to edit it manually in the XML/Glade file...

> I'll make sure not to commit this piece but it is something to watch out
> for if you start hitting errors locally.
>
>    
Right, well, it's the interface designer issue I guess. Better to edit 
it manually like I did before...
> - Cole
>    
Thanks,
- Michal