5:29 a.m.
On Wed, Nov 30, 2016 at 04:06:37PM +0530, prasanna.kalever(a)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/ :|