Re: [libvirt] PATCH: 14/28: Thread safety for network driver

[Daniel Veillard <veillard redhat com>]

On Sun, Nov 30, 2008 at 11:55:51PM +0000, Daniel P. Berrange wrote:
> This patch makes the network driver thread safe, following the pattern of
> the earlier patches

> @@ -95,6 +107,7 @@ networkAutostartConfigs(struct network_d
>      unsigned int i;

  Hum, you're not locking the driver itself during the loop ?
If a concurrent call were to remove one of the network object in
the meantime, that would lead to potentially random memory accesses
no ?

>      for (i = 0 ; i < driver->networks.count ; i++) {
> +        virNetworkObjLock(driver->networks.objs[i]);
>          if (driver->networks.objs[i]->autostart &&
>              !virNetworkIsActive(driver->networks.objs[i]) &&
>              networkStartNetworkDaemon(NULL, driver, driver->networks.objs[i]) < 0) {
> @@ -103,6 +116,7 @@ networkAutostartConfigs(struct network_d
>                         driver->networks.objs[i]->def->name,
>                         err ? err->message : NULL);
>          }
> +        virNetworkObjUnlock(driver->networks.objs[i]);
>      }
>  }
>  

[...]
>  cleanup:
> +    if (network)
> +        virNetworkObjUnlock(network);

  like for domain, I would suggest the unlocking accessor to accept NULL
and drop all the if constructs

  If there is a good reason networkAutostartConfigs() doesn't need to
lock the top level driver lock, okay, otherwise +1 after fix,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/