Re: [libvirt] PATCH: 13/25: Remove global state from XM driver

"Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: > The 'xm' driver currently keeps all its state in a global static > variables. Not cool, since we access this from all sorts of places > and its hard to guarentee thread safety. So we move the state into > the virConnect object. This will increase memory usage if a single > process has multiple Xen connections open though. > > Undecided whether this is a big problem or not. If so, I'll can > try and redo the next thread locking patch to use a lock over the > existing global state. Looks fine, modulo a potential NULL dereference and an obsolete comment. ... > typedef struct _xenUnifiedPrivate *xenUnifiedPrivatePtr; > diff --git a/src/xm_internal.c b/src/xm_internal.c ... > @@ -615,14 +586,12 @@ xenXMOpen (virConnectPtr conn ATTRIBUTE_ > * Free the config files in the cache if this is the > * last connection > */ It'd be good to remove or reword the comment above. Mentioning "last connection" doesn't make sense anymore, since the hash table is now per-connection. > -int xenXMClose(virConnectPtr conn ATTRIBUTE_UNUSED) { > - nconnections--; > - if (nconnections <= 0) { > - virHashFree(nameConfigMap, NULL); > - nameConfigMap = NULL; > - virHashFree(configCache, xenXMConfigFree); > - configCache = NULL; > - } > +int xenXMClose(virConnectPtr conn) { Shouldn't this function be "static"? Same with xenXMOpen.

> @@ -1310,6 +1282,7 @@ no_memory: > * domain, suitable for later feeding for virDomainCreateXML > */ > char *xenXMDomainDumpXML(virDomainPtr domain, int flags) { > + xenUnifiedPrivatePtr priv = domain->conn->privateData; dereferencing "domain" here can't be right, because immediately after the above line, we see these tests for NULL domain and domain->conn: if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { xenXMError((domain ? domain->conn : NULL), VIR_ERR_INVALID_ARG, __FUNCTION__); return(NULL); } Besides, "priv" is already initialized in the next hunk (below). IME, best is just to move the declaration down to the first use. That's better from a maintainability standpoint, since then there's no risk of accidentally using "priv" between the declaration and first use.