Re: [libvirt] [PATCH v4] virsh: Add option to undefine storage with domains
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