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(a)redhat.com> wrote:
> 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
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/ :|