"Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
> + xenUnifiedPrivatePtr priv = conn->privateData;
> +
> + virHashFree(priv->nameConfigMap, NULL);
> + virHashFree(priv->configCache, xenXMConfigFree);
> +
> return (0);
> }
> @@ -631,6 +600,7 @@ int xenXMClose(virConnectPtr conn ATTRIB
> * VCPUs and memory.
> */
> int xenXMDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info) {
> + xenUnifiedPrivatePtr priv;
> const char *filename;
> xenXMConfCachePtr entry;
> if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) {
> @@ -642,10 +612,12 @@ int xenXMDomainGetInfo(virDomainPtr doma
> if (domain->id != -1)
> return (-1);
>
> - if (!(filename = virHashLookup(nameConfigMap, domain->name)))
> + priv = domain->conn->privateData;
> +
> + if (!(filename = virHashLookup(priv->nameConfigMap, domain->name)))
> return (-1);
>
> - if (!(entry = virHashLookup(configCache, filename)))
> + if (!(entry = virHashLookup(priv->configCache, filename)))
> return (-1);
>
> memset(info, 0, sizeof(virDomainInfo));
> @@ -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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list