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

[Jim Meyering]

"Daniel P. Berrange" <berrange at redhat.com> 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.