5:49 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org