On 12/13/2011 05:44 AM, Peter Krempa wrote:
Add an option for virsh undefine command, to remove associated storage volumes while undefining a domain. This patch alows the user to remove
s/alows/allows/
associated (libvirt managed ) storage volumes while undefining a domain.
The new option --storage for the undefine command takes a string argument that consists of comma separated list of target or source path of volumes to be undefined. Volumes are removed after the domain has been successfuly undefined,
s/successfuly/successfully/
If a volume is not part of a storage pool, the user is warned to remove the volume in question himself.
Option --wipe-storage may be specified along with this, that ensures the image is wiped before removing.
Option --remove-all-storage enables the user to remove all storage. The name is chosen long as the users shoudl be aware what they're about to
s/shoudl/should/
do. --- Changes to v2 based on reveiw by Eric: ( http://www.redhat.com/archives/libvir-list/2011-September/msg00481.html ) - First undefine the domain, then undefine volumes - Add option to remove all volumes - Fix typos and strange wordings - Change retur value to failure if some operations on volumes fail.
It's been a while - I had flushed it from my mental cache in the meantime :)
@@ -1965,6 +1991,18 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false;
+ /* check if a string that should contain list of volumes to remove is present */ + if (vshCommandOptString(cmd, "storage", (const char **)&volumes) > 0) { + /* caution volumes has to be re-assigned as it would be doulbe freed */
s/doulbe/double/ I agree that we have to malloc the end-result into volumes, as strtok_r modifies the string but we are not allowed to modify the result returned by vshCommandOptString. Hmm, I'd almost rather introduce a second, temporary variable in order to avoid the casting and the confusing re-assignment, as in: if (vshCommandOptString(cmd, "storage", &storage) > 0) { volumes = vshStrdup(ctl, storage);
+ + for (i = 0; i < nvolumes; i++) {
+ + if (volumes) { + volume = strtok_r(volumes, ",", &saveptr);
Oops - strtok_r modifies volumes, which means after the first iteration through the outer for loop, you've corrupted it. I think you need to do the strtok_r loop breaking volumes into a char** array prior to entering the for loop.
+ + if (!(vol = virStorageVolLookupByPath(ctl->conn, source))) { + vshPrint(ctl, + _("Storage volume '%s' is not managed by libvirt. " + "Remove it manually.\n"), source);
Suppose the user passed in --storage=/path/to/disk; this message still outputs "Storage volume 'vda' is not managed...". It would be nicer to reuse the user's naming.
=item B<undefine> I<domain-id> [I<--managed-save>] [I<--snapshots-metadata>] +[I<--storage> B<volumes> I<--wipe-storage> I<--remove-all-storage>]
I'd format this as: [{I<--storage B<volumes> | I<--remove-all-storage>} [I<--wipe-storage]]
Undefine a domain. If the domain is running, this converts it to a transient domain, without stopping it. If the domain is inactive, @@ -1194,6 +1195,23 @@ domain. Without the flag, attempts to undefine an inactive domain with snapshot metadata will fail. If the domain is active, this flag is ignored.
+The I<--storage> flag takes a parameter B<volumes>, that is a comma separated +list of volume target names or source paths of storage volumes to be removed +along with the undefined domain. Volumes can be undefined and thus removed only +on inactive domains. Volume deletion is only attempted after the domain is +undefined; if not all of the requested volumes could be deleted, the the +error message indicates what still remains behind. If a volume path is not +found in the domain definition, it's treated as if the volume was successfuly
s/successfuly/successfully/
+deleteted.
s/deleteted/deleted/
+(See B<domblklist> for list of target names associated to a domain). +Example: --storage vda,vdb,vdc
Maybe make it obvious that both formats are possible: Example: --storage vda,/path/to/images/data.img
+ +The I<--remove-all-storage> flag specifies, that all domain's storage volumes
s/,// s/domain's/of the domain's/
+should be deleted. + +The flag I<--wipe-storage> specifies that the storage volumes should be +wiped before removal.
Looks a lot better, but I'm still worried about the correct handling of 'volumes', so I'd be more comfortable seeing a v4. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org