[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]