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

[Cole Robinson]

On 11/22/2017 07:40 AM, Lin Ma wrote:
> 
> 
>>>> 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?
> 

Hmm my mistake, you are correct. Forgot about address type='drive',
didn't realized it was used for all disk devices.

>>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?
>  

Yes that looks good

> 
>>>                      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?
> 

Hmm. lsscsi and links in /sys/bus/scsi/devices/ seem to use
$host:$bus:$target:$lun formating. lshw uses $host:$bus.$target.$lun. So
I guess either of those is fine

Thanks,
Cole

> 
>>> +            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