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

[Cole Robinson]

On 10/03/2013 05:07 PM, anonym wrote:
> 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).
> 

Ah, sorry I missed a bit in your first patch. But what you want to do is
follow the pattern of OSXML.enable_bootmenu and the corresponding
enable_bootmenu in cli.py. It's a similar situation where absent = default,
and a present XML value is explicitly on/off

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

Some are probably mistakes. But most share the UI init with addhardware.py
which doesn't apply in this case.

>>> [...]
>>> +        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).
> 

'default' is the same as 'not listed at all', so if we unconditionally showed
it for the 'default' value, that means we show it all the time for every
hypervisor.

What I meant was: show it for hypervisors that we know support it, _and_ if
our whitelist is wrong but there's an explicit value set, show it. Just to
prevent some future hypothetical case of someone saying 'hey I set
removeable=false in the XML but I don't see it in the UI'. It's unlikely to
happen in practice so don't worry about it :)

Thanks,
Cole