Re: [libvirt] [PATCH] repeat lookup by name in LookupByID
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/