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

[Daniel P. Berrange]

On Mon, Jan 19, 2009 at 04:07:33PM +0100, Jim Meyering wrote:
> "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.

Probably - for historical reasons many of the Xen functions are
pre-declared in the header files, because they used to be called
directly before the days of the generic driver API. Should make
most of them static now really.

> > @@ -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.

The giant if (....) chcek for NULL here is really totally unneccessary.
This method is only ever called from the driver APIs and the caller
already does exactly this check, so repeating it is just cluttering up
the code.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|