On Wed, Nov 30, 2016 at 04:06:37PM +0530, prasanna.kalever@xxxxxxxxxx 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/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
