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

[anonym]

03/10/13 23:20, Cole Robinson wrote:
> 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

Yeah, I figured it out (hence my previous email -- I missed this one).

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

I didn't mean we should show it for the 'default' value, rather that
we'd not show it at all if the hypervisor doesn't support it. The reason
for this is...

> 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 :)

... that I imagine the user will say 'hey, where did the check box go?'
after he or she unchecks it and applies the setting. :)

Any way, I threw it in as a separate patch. The patch set should be sent
any minute.

Cheers!