Re: [PATCH] virsh: Return false if only '--wipe-storage' is assigned when undefine a domain

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

 



On 05/14/2014 12:15 AM, Li Yang wrote:

Long subject line; it's better to keep subject under 60 columns, for the
sake of 'git shortlog' in an 80-column window.  I went with:

virsh: reject undefine --wipe-storage without also naming storage

> For now, if only '--wipe-storage' is assigned, user can undefine a
> domain normally. But actually '--wipe-storage' doesn't work, this

More that it silently does nothing, not that it doesn't work.

> may confuse user. And since '--wipe-storage' wipes data on the
> removed volumes, if no removed volume storage assigned, we'd better
> raise an error message.
> 
> Before:
> $ virsh undefine virt-tests-vm1 --wipe-storage
> Domain virt-tests-vm1 has been undefined
> 
> After:
> $ virsh undefine virt-tests-vm1 --wipe-storage
> error: '--wipe-storage' needs storage volume deletion: '--stroage <string>' or '--remove-all-storage' is necessary.

s/stroage/storage/

> 
> Signed-off-by: Li Yang <liyang.fnst@xxxxxxxxxxxxxx>
> ---
>  tools/virsh-domain.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 3a7c260..25236a0 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -2982,6 +2982,14 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>  
>      ignore_value(vshCommandOptString(cmd, "storage", &vol_string));
>  
> +    if (!(vol_string || remove_all_storage) && wipe_storage) {
> +        vshError(ctl,
> +                 _("'--wipe-storage' needs storage volume deletion: "
> +                   "'--stroage <string>' or '--remove-all-storage' "
> +                   "is necessary."));

We tend to avoid trailing '.' in error messages (although we aren't
consistent).  This is also long-winded and has a typo.

ACK to the idea, so I'm pushing with this modification:

diff --git i/tools/virsh-domain.c w/tools/virsh-domain.c
index a5ddb93..84a6706 100644
--- i/tools/virsh-domain.c
+++ w/tools/virsh-domain.c
@@ -2979,14 +2979,12 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
     size_t i;
     size_t j;

-
     ignore_value(vshCommandOptString(cmd, "storage", &vol_string));

     if (!(vol_string || remove_all_storage) && wipe_storage) {
         vshError(ctl,
-                 _("'--wipe-storage' needs storage volume deletion: "
-                   "'--stroage <string>' or '--remove-all-storage' "
-                   "is necessary."));
+                 _("'--wipe-storage' requires '--storage <string>' or "
+                   "'--remove-all-storage'"));
         return false;
     }



-- 
Eric Blake   eblake redhat com    +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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]