[libvirt] [PATCH v3] virsh: Add option to undefine storage with domains

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111213/bacd7aa0/attachment-0001.sig>