On Wed, Nov 30, 2016 at 05:47:46PM +0530, Prasanna Kalever wrote:
On Wed, Nov 30, 2016 at 4:59 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Wed, Nov 30, 2016 at 04:06:37PM +0530, prasanna.kalever@redhat.com wrote:
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8e86704..18b7fc3 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -31,11 +31,34 @@ #include "virstoragefile.h" #include "virstring.h" #include "viruri.h" +#include "viratomic.h"
#define VIR_FROM_THIS VIR_FROM_STORAGE
VIR_LOG_INIT("storage.storage_backend_gluster");
+static bool virGlusterGlobalError; +static virOnceControl glusterConnOnce = VIR_ONCE_CONTROL_INITIALIZER; + +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState; +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr; + +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; + +typedef struct _unixSocketAddress unixSocketAddress; + +typedef struct _inetSocketAddress inetSocketAddress; + +typedef struct _glusterServer glusterServer; +typedef struct _glusterServer *glusterServerPtr;
*all* structs should be fully namespaced. ie have a VirStorageBackendGluster name prefix
Thanks for correcting me here.
+struct _virStorageBackendGlusterStatePreopened { + virStorageFileBackendGlusterPrivPtr priv; + unsigned int nservers;
size_t
+ glusterServerPtr *hosts; + char *volname; + volatile int ref; +}; + +struct _virStorageBackendGlusterconnCache { + virMutex lock; + size_t nConn; + virStorageBackendGlusterStatePtrPreopened *conn; +}; + +virStorageBackendGlusterconnCachePtr connCache = {0,};
'static'
and all static vars are initialized to zero so kill {0,}
yeah, pretty straight!
+static void +virGlusterConnAlloc(void) +{ + if ((VIR_ALLOC(connCache) < 0)) + virGlusterGlobalError = false; +} + +static int +virStorageBackendGlusterSetPreopened(virStorageSourcePtr src, + virStorageFileBackendGlusterPrivPtr priv) +{ + unsigned int idx; + virStorageBackendGlusterStatePtrPreopened entry = NULL; + + if (connCache == NULL && (virOnce(&glusterConnOnce, + virGlusterConnAlloc) < 0)) + return -1;
Checking if connCache is NULL before calling virOnce is pointless. The whole point of virOnce is that you can blindly call it multiple times and it'll only run once.
Perfectly make sense.
+ if (virGlusterGlobalError) + return -1; + + if (!connCache->nConn) { + if (virMutexInit(&connCache->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + return -1; + } + }
This is not thread safe - mutex initialization should be in the virOnce intializer.
If we try to keep this in virOnce intializer (executed only once during process life time), then we end up not having a virMutexDestroy(). Is that okay ?
It is required - this is global state that needs to exist forever. If you try to destroy the mutex you'll create thread safety problems. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|