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