[libvirt] [PATCH v4] virsh: Add option to undefine storage with domains

[Peter Krempa]

On 12/15/2011 09:17 PM, Eric Blake wrote:
> On 12/15/2011 08:20 AM, Peter Krempa wrote:
>> Changes to v3:
>>          - fix ton'o'typos (I should install a spell checker.)
>>          - add a new variable to hold copy of argument to avoid confusion,
>>            typecasting ...
>>          - break volume string into tokens just once and store it in an array
>>          - add both target and source as volume description
>>            ( both are needed, as the user should know which volume he shoud
>
> Dare I mention the typo here?  Nah, since it won't be part of the final
> commit.  :)

Dang. Happens _every_ time :(

>
>> @@ -2027,6 +2066,20 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>>           snapshots_safe = true;
>>       }
>>
>> +    /* Stash domain description for later use */
>> +    if (remove_storage || remove_all_storage) {
>> +        if (running) {
>> +            vshError(ctl, _("Storage volume deletion is supported only on stopped domains"));
>> +            goto cleanup;
>> +        }
>
> Doesn't hold up this patch, but I wonder if we should have a counterpart
> virsh operation that allows deletion of volumes after destroying a
> transient domain.  On the other hand, virsh isn't used as often with
> transient domains, and any management app (I'm thinking things like
> VDSM) that already prefers transient domains probably already knows how
> to properly wipe storage after the fact, without needing syntactic sugar
> in virsh.

Well, I think that would require remembering that the user wishes to 
remove storage in (transient) domain's configuration, and when the 
daemon stops the domain, it would remove the storage. Or virsh would 
have to wait if the domain disappears and then undefine the storage, but 
this would have limited usability as it would require virsh to run while 
the transient domain is being destroyed.

>>
>>   =item B<undefine>  I<domain-id>  [I<--managed-save>] [I<--snapshots-metadata>]
>> +[I<--storage>  B<volumes>  | I<--remove-all-storage>  I<--wipe-storage>]
>
> We want to make it obvious that --wipe-storage applies only if you use
> either --storage or --remove-all-storage, so this has to be:
>
> [ {I<--storage>  B<volume>  | I<--remove-all-storage>} [I<--wipe-storage]]
>
> The outer [] says storage removal in general is optional, then the first
> {} says that storage removal is an either-or between the two styles, and
> the second [] says that --wipe-storage goes with either style.

I see your point. This is this definitely the best option to write it, 
but I somehow overlooked the curly braces while reading your v3 review :/

>
> ACK with nits in virsh.pod fixed; no need to send a v5.
>

Thanks, fixed and pushed.

Peter