On Wed, Jul 16, 2008 at 05:42:57PM +0100, Daniel P. Berrange wrote:

On Wed, Jul 16, 2008 at 12:04:39PM +0400, Evgeniy Sokolov wrote:

...

There was error every time when I undefine stoped container "no domain with matching id". Bug arrise due to stoped container has ID = -1.

This is not an error - this is intentional. There is no meaningful ID for an inactive domain, thus lookupByID is intended to fail.

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.

...

In such case container will be searched by name.

No, this is wrong. The application should use lookupByName instead.

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.

...

other: use VIR_ERR_NO_DOMAIN when domain is not found

...

Index: src/openvz_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/openvz_driver.c,v retrieving revision 1.28 diff -u -p -r1.28 openvz_driver.c --- src/openvz_driver.c 11 Jul 2008 11:09:44 -0000 1.28 +++ src/openvz_driver.c 16 Jul 2008 07:51:19 -0000 @@ -128,11 +128,22 @@ static void cmdExecFree(char *cmdExec[]) static virDomainPtr openvzDomainLookupByID(virConnectPtr conn, int id) { struct openvz_driver *driver = (struct openvz_driver *)conn->privateData; - struct openvz_vm *vm = openvzFindVMByID(driver, id); + struct openvz_vm *vm; virDomainPtr dom;

+ vm = openvzFindVMByID(driver, id); + + if (!vm) { /*try to find by name*/ + char name[OPENVZ_NAME_MAX]; + if (snprintf(name, OPENVZ_NAME_MAX, "%d",id) >= OPENVZ_NAME_MAX) { + openvzError(conn, VIR_ERR_INTERNAL_ERROR, _("Too long domain name")); + return NULL; + } + vm = openvzFindVMByName(driver, name); + }

This souldn't be applied.

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@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:

Daniel Veillard ?????:

...

On Wed, Jul 16, 2008 at 08:10:21PM +0100, Daniel P. Berrange wrote:

...

Yes, the documentation is wrong - all inactive VMs have an ID of -1, and thus lookup-by-ID is nonsensical for inactive VMs.

If any application did make use of this change which falls back to lookup-by-name, then it would only ever work with OpenVZ and not any of the other libvirt drivers, which isn't useful behaviour. [...]

...

Then the virLookupById description must be updated, I'm not against it, but we need to be coherent. Indeed, the docs need to be clarified.

okay, what about * Try to find a domain based on the hypervisor ID number * Note that this won't work for inactive domains which have an ID of -1, * in that case a lookup based on the Name or UUId need to be done instead. Ok. In that case we may disable lookup-by-id in undefine subcommand.

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.

Index: virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.155 diff -u -p -r1.155 virsh.c --- virsh.c 29 May 2008 14:56:12 -0000 1.155 +++ virsh.c 17 Jul 2008 09:04:17 -0000 @@ -978,7 +978,8 @@ cmdUndefine(vshControl * ctl, vshCmd * c if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE;

- if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) + if (!(dom = vshCommandOptDomainBy(ctl, cmd, "domain", &name, + VSH_BYNAME|VSH_BYUUID))) return FALSE;

if (virDomainUndefine(dom) == 0) {

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@redhat.com> wrote:

On Thu, Jul 17, 2008 at 01:20:59PM +0400, Evgeniy Sokolov wrote: ...

...

Index: virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.155 diff -u -p -r1.155 virsh.c --- virsh.c 29 May 2008 14:56:12 -0000 1.155 +++ virsh.c 17 Jul 2008 09:04:17 -0000 @@ -978,7 +978,8 @@ cmdUndefine(vshControl * ctl, vshCmd * c if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE;

- if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", &name))) + if (!(dom = vshCommandOptDomainBy(ctl, cmd, "domain", &name, + VSH_BYNAME|VSH_BYUUID)))

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 d8a30f1f4da5db63b0c3cef2a67183a76c7dc989 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> 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