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
+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,}
+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.
+ 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.
+ + for (idx = 0; idx < connCache->nConn; idx++) { + if (STREQ(connCache->conn[idx]->volname, src->volume)) + return 0; + }
Not thread safe since connCache is not locked at this time
+ + if (VIR_ALLOC(entry) < 0) + return -1; + + if (VIR_STRDUP(entry->volname, src->volume) < 0) + goto error; + + if (VIR_ALLOC_N(entry->hosts, entry->nservers) < 0) + goto error; + + entry->nservers = src->nhosts; + + for (idx = 0; idx < src->nhosts; idx++) { + if (VIR_ALLOC(entry->hosts[idx]) < 0) + goto error; + if (src->hosts[idx].transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (VIR_STRDUP(entry->hosts[idx]->u.uds.path, + src->hosts[idx].socket) < 0) + goto error; + entry->hosts[idx]->type = VIR_STORAGE_NET_HOST_TRANS_UNIX; + } else if (src->hosts[idx].transport == + VIR_STORAGE_NET_HOST_TRANS_TCP) { + if (VIR_STRDUP(entry->hosts[idx]->u.tcp.host, + src->hosts[idx].name) < 0) + goto error; + if (VIR_STRDUP(entry->hosts[idx]->u.tcp.port, + src->hosts[idx].port) < 0) + goto error; + entry->hosts[idx]->type = VIR_STORAGE_NET_HOST_TRANS_TCP; + } else { + entry->hosts[idx]->type = VIR_STORAGE_NET_HOST_TRANS_LAST; + } + } + entry->priv = priv; + + virMutexLock(&connCache->lock); + virAtomicIntSet(&entry->ref, 1); + if (VIR_INSERT_ELEMENT(connCache->conn, -1, connCache->nConn, entry) < 0) { + virMutexUnlock(&connCache->lock); + goto error; + } + virMutexUnlock(&connCache->lock); + + return 0; + + error: + for (idx = 0; idx < entry->nservers; idx++) { + if (entry->hosts[idx]->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + VIR_FREE(entry->hosts[idx]->u.uds.path); + } else { + VIR_FREE(entry->hosts[idx]->u.tcp.host); + VIR_FREE(entry->hosts[idx]->u.tcp.port); + } + VIR_FREE(entry->hosts[idx]); + } + + VIR_FREE(entry->hosts); + VIR_FREE(entry->volname); + VIR_FREE(entry); + + return -1; +} + +static glfs_t * +virStorageBackendGlusterFindPreopened(virStorageSourcePtr src) +{ + unsigned int cIdx, sIdx, nIdx; /* connectionIdx, savedIdx, newIdx */ + bool flag = false; + + if (connCache == NULL) + return NULL;
Not thread safe, since something could be in middlle of calling the virOnce to initialize this.
+ + virStorageBackendGlusterStatePtrPreopened entry; + + for (cIdx = 0; cIdx < connCache->nConn; cIdx++) { + entry = connCache->conn[cIdx];
You've not locked connCache, and same in later methods
+ if (STREQ(entry->volname, src->volume)) { + if (entry->nservers == src->nhosts) { + for (sIdx = 0; sIdx < entry->nservers; sIdx++) { + for (nIdx = 0; nIdx < src->nhosts; nIdx++) { + if (entry->hosts[sIdx]->type == + src->hosts[nIdx].transport) { + if (entry->hosts[sIdx]->type + == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (STREQ(entry->hosts[sIdx]->u.uds.path, + src->hosts[nIdx].socket)) { + flag = true; + break; + } + } else { + if (STREQ(entry->hosts[sIdx]->u.tcp.host, + src->hosts[nIdx].name) && + STREQ(entry->hosts[sIdx]->u.tcp.port, + src->hosts[nIdx].port)) { + flag = true; + break; + } + } + } + } + if (!flag) + return NULL; + flag = false; + } + virAtomicIntInc(&entry->ref); + return entry->priv->vol; + } else { + return NULL; + } + } + } + return NULL; +} 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/ :|