[libvirt] PATCH: 16/28: Thread safety for storage driver

[Daniel Veillard]

On Sun, Nov 30, 2008 at 11:57:47PM +0000, Daniel P. Berrange wrote:
> This patch makes the storage driver thread safe
[...]
>  static void
>  storageDriverAutostart(virStorageDriverStatePtr driver) {

  Hum, there is something I find incherent, in the network
side we don't lock the driver but we lock the individual
objects in the autostart. Here we don't lock anything. In both
case they are started at the end of the Startup method, which
does lock the driver. So it seems in the network case we
don't need to lock the individual objects unless I'm mistaken

> @@ -172,11 +182,13 @@ storageDriverReload(void) {
>      if (!driverState)
>          return -1;
>  
> +    storageDriverLock(driverState);
>      virStoragePoolLoadAllConfigs(NULL,
>                                   &driverState->pools,
>                                   driverState->configDir,
>                                   driverState->autostartDir);
>      storageDriverAutostart(driverState);
> +    storageDriverUnlock(driverState);
>  
>      return 0;

  Hum there is something potentially nasty here:
    - suppose you call reload
    - you lock the main driver
    - you don't lock any of the individual objects
    - another thread is busy on a long standing operation in one of the
      storage objects
    - reload still goes in, virStoragePoolDefParse creates a brand new
      object and virStoragePoolObjAssignDef frees the object used by the
      other thread.

I didn't find any locking in virStoragePoolObjAssignDef or
virStoragePoolLoadAllConfigs. Maybe on reload operation it's safer to
just lock the driver and all storage objects before reloading all configs
and autostarting.

[...]
> @@ -393,6 +440,8 @@ storageListDefinedPools(virConnectPtr co
>      return -1;
>  }
>  
> +/* This method is required to be re-entrant / thread safe, so
> +   uses no driver lock */

  Well virStorageBackendFindPoolSources prototype has no comment so it's
  hard to guess...

  Okay, there is a couple issues I raise here, I'm not sure if it's
  misunderstanding or genuine problems though...

Daniel

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