[libvirt] [PATCH] virsh: Return false if only '--wipe-storage' is assigned when undefine a domain
For now, if only '--wipe-storage' is assigned, user can undefine a domain normally. But actually '--wipe-storage' doesn't work, this 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. Signed-off-by: Li Yang <liyang.fnst@cn.fujitsu.com> --- 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.")); + return false; + } + if (managed_save) { flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE; managed_save_safe = true; -- 1.7.1
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@cn.fujitsu.com> --- 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
participants (2)
-
Eric Blake -
Li Yang