On Wed, Jul 16, 2008 at 05:42:57PM +0100, Daniel P. Berrange wrote: Hum ... the description just says: "Try to find a domain based on the hypervisor ID number" it's true that for most implementations the ID is only usable on running domains, but on OpenVZ it's not the case, the ID is persistant when the domain stopped, since that's the only identifier with the UUID. Usually as a far more expensive fallback like for lookupByUUID. If we reverse the patch then we should update the virLookupById description to state that it will work only on running domains. I'm not sure it's really a gain in general. Application should fallback to Name or UUID lookups If the virLookupById semantic is that it should work only on running domain then we can expect the client code to behave accordingly only if we document it. Then the virLookupById description must be updated, I'm not against it, but we need to be coherent. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Thu, Jul 17, 2008 at 01:20:59PM +0400, Evgeniy Sokolov wrote: Ahhhh, so that's why you were seeing the error. Yes, this makes sens because a VM has to be shutoff before undefine is allowed. ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: ... Before this change, we'd get a hint that undefining a running domain is not possible: $ virsh -q -c test:///default undefine 1 virsh # libvir: Test error test: internal error Domain is still running error: Failed to undefine domain 1 virsh # Now, the hint is gone, and the new diagnostics are misleading, since domain "1" certainly does exist: $ ./virsh -q -c test:///default undefine 1 virsh # libvir: Test error : Domain not found error: failed to get domain '1' virsh # With the patch below virsh tells what's wrong: $ ./virsh -q -c test:///default undefine 1 error: a running domain like 1 cannot be undefined; to undefine, first shutdown then undefine using its name or UUID [Exit 1] My first attempt called vshCommandOptDomainBy-with-BYID only upon failure of the with-VSH_BYNAME|VSH_BYUUID call, but then you'd still get this misleading diagnostic: error: failed to get domain '1' It also adds a test that essentially does this: $ ./virsh -q -c test:///default undefine 1 error: a running domain like 1 cannot be undefined; to undefine, first shutdown then undefine using its name or UUID [Exit 1] $ ./virsh -q -c test:///default undefine test libvir: Test error test: internal error Domain is still running error: Failed to undefine domain test [Exit 1] $ ./virsh -q -c test:///default 'shutdown 1; undefine test' Domain 1 is being shutdown Domain test has been undefined From: Jim Meyering <meyering(a)redhat.com&gt; Date: Fri, 18 Jul 2008 10:53:25 +0200 Subject: [PATCH] better diagnostic when failing to undefine a running domain via ID * src/virsh.c (cmdUndefine): Tell user to shutdown and then use name or UUID. * tests/undefine: New test. Exercise virsh's undefine command. * tests/Makefile.am (test_scripts): Add undefine. --- src/virsh.c | 14 +++++++++++++ tests/Makefile.am | 1 + tests/undefine | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 0 deletions(-) create mode 100755 tests/undefine diff --git a/src/virsh.c b/src/virsh.c index f1296ec..620f103 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -974,10 +974,24 @@ cmdUndefine(vshControl * ctl, vshCmd * cmd) virDomainPtr dom; int ret = TRUE; char *name; + int found; + int id; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; + name = vshCommandOptString(cmd, "domain", &found); + if (!found) + return FALSE; + + if (name && virStrToLong_i(name, NULL, 10, &id) == 0 + && id >= 0 && (dom = virDomainLookupByID(ctl->conn, id))) { + vshError(ctl, FALSE, _("a running domain like %s cannot be undefined;\n" + "to undefine, first shutdown then undefine" + " using its name or UUID"), name); + virDomainFree(dom); + return FALSE; + } if (!(dom = vshCommandOptDomainBy(ctl, cmd, "domain", &name, VSH_BYNAME|VSH_BYUUID))) return FALSE; diff --git a/tests/Makefile.am b/tests/Makefile.am index 2fc5a2f..5686678 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -51,6 +51,7 @@ test_scripts += \ int-overflow \ read-bufsiz \ read-non-seekable \ + undefine \ vcpupin endif diff --git a/tests/undefine b/tests/undefine new file mode 100755 index 0000000..3936e22 --- /dev/null +++ b/tests/undefine @@ -0,0 +1,55 @@ +#!/bin/sh +# exercise virsh's "undefine" command + +# Copyright (C) 2008 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>;. + +if test "$VERBOSE" = yes; then + set -x + virsh --version +fi + +. $srcdir/test-lib.sh + +fail=0 + +# Attempt to undefine a running domain, by domain name. +virsh -q -c test:///default undefine test > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +libvir: Test error test: internal error Domain is still running +error: Failed to undefine domain test +EOF +compare out exp || fail=1 + +# A different diagnostic when specifying a domain ID +virsh -q -c test:///default undefine 1 > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: a running domain like 1 cannot be undefined; +to undefine, first shutdown then undefine using its name or UUID +EOF +compare out exp || fail=1 + +# Succeed, now: first shut down, then undefine, both via name. +virsh -q -c test:///default 'shutdown test; undefine test' > out 2>&1 +test $? = 0 || fail=1 +cat <<\EOF > exp || fail=1 +Domain test is being shutdown +Domain test has been undefined +EOF +compare out exp || fail=1 + +(exit $fail); exit $fail -- 1.5.6.3.440.ge3a8d