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@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list