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@xxxxxxxxxx> wrote:
> > 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
>
> 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/ :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
