[libvirt] [PATCH] repeat lookup by name in LookupByID

[Daniel P. Berrange]

On Wed, Jul 16, 2008 at 02:31:18PM -0400, Daniel Veillard wrote:
> 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.

The distinction here is that we actually have several different
identifiers, with varying semantics

 - the libvirt ID associated with a virDomainObject
 - the libvirt name associated with a virDomainObject
 - the OpenVZ  ID associated with a domain

The OpenVZ ID is used as the basis for both libvirt ID and libvirt
name. Even though the OpenVZ  ID is available for inactive domains,
this does not mean the  libvirt ID needs to be filled in for 
inactive domains.
> 
> > > 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.

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.

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

Indeed, the docs need to be clarified.

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