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.

> > 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.