[virt-tools-list] 答复： Re: [virt-manager PATCH 3/7] details: Show attached disk information in scsi controller page

[Lin Ma]

>>> Cole Robinson <crobinso at redhat.com> 2017/11/22 星期三 上午 6:28 >>>
>On 11/06/2017 07:52 AM, Lin Ma wrote:
>> It helps users to recognize controllers <-> disks mapping relationship.
>> 
>> Signed-off-by: Lin Ma <lma at suse.com>
>> ---
>>  ui/details.ui   	   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  virtManager/details.py | 25 ++++++++++++++++++++++++-
>>  2 files changed, 72 insertions(+), 1 deletion(-)
>> 
>> diff --git a/virtManager/details.py b/virtManager/details.py
>> index 22e0786e..a9e0cb79 100644
>> --- a/virtManager/details.py
>> +++ b/virtManager/details.py
>> @@ -1042,6 +1042,16 @@ class vmmDetails(vmmGObjectUI):
>>		  uiutil.init_combo_text_column(combo, 1)
>>		  combo.set_active(-1)
>>  
>> +	    attached_disk_list = self.widget("scsi-disk-list")
>> +	    model = Gtk.ListStore(str)
>> +	    attached_disk_list.set_model(model)
>> +	    attached_disk_list.set_headers_visible(False)
>> +	    diskTextCol = Gtk.TreeViewColumn()
>> +	    disk_txt = Gtk.CellRendererText()
>> +	    diskTextCol.pack_start(disk_txt, True)
>> +	    diskTextCol.add_attribute(disk_txt, 'text', 0)
>> +	    attached_disk_list.append_column(diskTextCol)
>> +
>>  
>
>Can you generalize all the naming here? This UI could easily be reused
>for all attached controller devices, let's not add 'disk' into all the
>naming that just needs to be changed later. So "controller-device-list"
>for the UI name, just name the variables combo/model/col/text here since
>it's just init code.
will do.

>Also in the UI change the label from 'Disks:' to 'Devices:'
ok
 

>>	  ##########################
>>	  # Window state listeners #
>> @@ -2995,17 +3005,30 @@ class vmmDetails(vmmGObjectUI):
>>		  if not dev:
>>			  return
>>  
>> +	    self.widget("disk-list-label").set_visible(False)
>> +	    self.widget("scsi-disk-box").set_visible(False)
>
>Change these names too. And you should be able to use
>uiutil.set_grid_row_visible(self.widget("XXX"), False) to hide the UI
ok, will change the corresponding names, and use uiutil.set_grid_row_visible instead.
 

>>		  can_remove = True
>>		  if self.vm.get_xmlobj().os.is_x86() and dev.type == "usb":
>>			  can_remove = False
>>		  if dev.type == "pci":
>>			  can_remove = False
>>		  if dev.type == "scsi":
>> +		    model = self.widget("scsi-disk-list").get_model()
>> +		    model.clear()
>>			  for disk in self.vm.get_disk_devices(inactive=True):
>>				  if (dev.type == disk.bus and
>>					  dev.index == disk.address.controller):
>
>Should have mentioned in the first patch. I think you should be checking
>disk.address.type here instead of disk.bus. Doesn't matter in practice
>but it more correctly identifies what you are checking for.
A scsi disk or a sata disk's address information looks like:
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
So the value of disk.address.type field is 'drive', I think we can't check it here.
 
Or use 'if (disk.address.type == "drive" and' instead of 'if (dev.type == disk.bus and'.
Does this make sense?

>Also if you move this to a function in VirtualDeviceAddress like
>compare_controller, we can use a central place to store logic for other
>devices as well.
I guess what you mean is:
	    if dev.type == "scsi":
		    model = self.widget("controller-device-list").get_model()
		    model.clear()
		    for disk in self.vm.get_disk_devices():
			    if disk.address.compare_controller(dev):
				    ......
am I right?
 

>>					  can_remove = False
>> -				    break
>> +				    name = _label_for_device(disk)
>> +				    bus = disk.address.bus
>> +				    target = disk.address.target
>> +				    unit = disk.address.unit
>> +				    infoStr = ("%s:  bus = %s, target = %s, unit = %s" %
>> +							   (name, bus, target, unit))
>> +				    model.append([infoStr])
>
>Can we just use _label_for_device instead of inventing a new label
>format? If that's no sufficient and the full address is desired I'd
>rather put some address pretty printing into virtinst and use that instead.
In my opinion, only _label_for_device is not sufficient, I lean to provide full
address information.
If we'll provide full address, I'll move them into a function in VirtualDeviceAddress.
could you please suggest an example of string format for pretty printing?
 

>> +		    self.widget("disk-list-label").set_visible(not can_remove)
>> +		    self.widget("scsi-disk-box").set_visible(not can_remove)
>> +
>
>I say always show the UI for scsi controllers, even if they have no
>disks attached.
ok, will do.
 
Thanks,
Lin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20171122/bda4e201/attachment.htm>