[virt-tools-list] [PATCH RFC] Add support for the new 'removable' disk flag.

[anonym]

03/10/13 18:09, Cole Robinson wrote:
> On 10/02/2013 10:39 AM, Fred A. Kemp wrote:
> For virt-install, don't worry about adding man page docs unless you want to.
> Extending the cli is pretty easy, you'll want to edit
> virtinst/cli.py:parse_disk, see parse_features acpi handling for an example of
> doing on|off.

Hmm. I assume you mean something like:

--- a/virtinst/cli.py
+++ b/virtinst/cli.py
@@ -1460,6 +1460,7 @@ def parse_disk(guest, optstr, dev=None,
validate=True):

     set_param("device", "device")
     set_param("bus", "bus")
+    set_param("removable", "removable", convert_cb=_on_off_convert)
     set_param("driver_cache", "cache")
     set_param("driver_name", "driver_name")
     set_param("driver_type", "driver_type")


This fails since it tries to set 'removable' in virtinst to True/False,
but I made it into a "on"/"off"/None tristate. It works for acpi since
it's a bool in virtinst. Perhaps what you mean in my next quote that you
want me to make 'removable' into a bool too (then it works)? If so,
there's an issue with the "future proof[ing]" of the check box for
'removable' in virt-manager you requested (see below).

> Also, for the UI, I realize that for libvirt this is more or less a tristate
> value of on/off/default, but since qemu is the only implementer so far, just
> use a checkbox in the UI and have default == off.

Ok, using a check box is certainly simpler than a combo box.

>> [...]
>> +        # Disk removable combo
>> +        disk_removable = self.widget("disk-removable")
>> +        uihelpers.build_disk_removable_combo(self.vm, disk_removable)
>> +
> 
> uihelpers should only be for shared code. since this UI isn't shared, open
> code that bit here.

Good to know. I just tried to follow what seemed the convention for
other combo boxes (they all have a build_*_combo in uihelpers, is that a
mistake?). It won't be necessary since we'll move to a checkbox, though.

>> [...]
>> +        self.widget("disk-removable").set_sensitive(is_usb and self.conn.is_qemu())
> 
> Rather than desensitive it, I'd prefer to hide it. You can do
> 
> can_set_removable = (is_usb and (self.conn.is_qemu() or self.conn.is_test_conn())
> uihelpers.set_grid_row_visible(self.widget("disk-removable"), can_set_removable)

Alright, thanks!

> Probably also want to show it if there's an explicit removable setting already
> set to future proof it a wee bit.

I assume by "explicit" you mean when 'removable' is set to 'on' or
'off', i.e. non-default? If so, I'll do it, but it feels a bit strange
as it fails when a device has the default value, which ought to be by
far the most probable scenario (the exception being when you've manually
edited the XML or similar before firing up virt-manager).

Also, as alluded to above, this will be impossible if you want me to
make 'removable' into a bool instead of the tristate. Due to the point
made in my previous paragraph, that isn't much of a problem, IMHO.

Or have I misunderstood you completely?

Cheers!