On 11/22/2017 07:40 AM, Lin Ma wrote:
>
>
>>>> Cole Robinson <crobinso@xxxxxxxxxx> 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@xxxxxxxx>
>>> ---
>>> 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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
