[libvirt] PATCH: 4/5: Locking in the LXC driver

[Daniel P. Berrange]

On Fri, Oct 17, 2008 at 07:53:06AM -0700, Dan Smith wrote:
> DB> @@ -104,8 +117,12 @@ static virDomainPtr lxcDomainLookupByID(
> DB>                                          int id)
> DB>  {
> DB>      lxc_driver_t *driver = (lxc_driver_t *)conn->privateData;
> DB> -    virDomainObjPtr vm = virDomainFindByID(&driver->domains, id);
> DB> +    virDomainObjPtr vm;
> DB>      virDomainPtr dom;
> DB> +
> DB> +    lxcDriverLock(driver);
> DB> +    vm = virDomainFindByID(&driver->domains, id);
> DB> +    lxcDriverUnlock(driver);
> 
> DB>      if (!vm) {
> DB>          lxcError(conn, NULL, VIR_ERR_NO_DOMAIN, NULL);
> DB> @@ -117,6 +134,7 @@ static virDomainPtr lxcDomainLookupByID(
> dom-> id = vm->def->id;
> DB>      }
> 
> DB> +    virDomainUnlock(vm);
> DB>      return dom;
> DB>  }
> 
> Can you explain why you're unlocking the vm in all of these functions
> without first the corresponding lock operation?

You didn't read the previous mail in this sequence :-P

  http://www.redhat.com/archives/libvir-list/2008-October/msg00419.html

[quote]
* Domain lookup methods

  virDomainObjPtr virDomainFindByID(const virDomainObjListPtr doms,
                                    int id);
  virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
                                      const unsigned char *uuid);
  virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
                                      const char *name);

  The driver must hold its global lock to protect the virDomainObjListPtr
  object. The returned virDomainObjPtr instance will be locked at which
  point the driver  can optionall release its global lock
[/quote]

> 
> DB> @@ -1091,17 +1230,20 @@ static int lxcShutdown(void)
> DB>   */
> DB>  static int
> DB>  lxcActive(void) {
> DB> -    unsigned int i;
> DB> +    unsigned int i, active = 0;
> 
> DB>      if (lxc_driver == NULL)
> DB>          return(0);
> 
> DB> -    for (i = 0 ; i < lxc_driver->domains.count ; i++)
> DB> +    lxcDriverLock(lxc_driver);
> DB> +    for (i = 0 ; i < lxc_driver->domains.count ; i++) {
> DB> +        virDomainLock(lxc_driver->domains.objs[i]);
> DB>          if (virDomainIsActive(lxc_driver->domains.objs[i]))
> DB> -            return 1;
> DB> +            active = 1;
> DB> +        virDomainUnlock(lxc_driver->domains.objs[i]);
> DB> +    }
> 
> DB> -    /* Otherwise we're happy to deal with a shutdown */
> DB> -    return 0;
> DB> +    return active;
> DB>  }
> 
> It looks to me like you're taking the driver lock and not releasing it
> on exit.

Yes, well spotted.

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