[libvirt] [PATCH 1/3] Domain events - primary implementation

[Daniel P. Berrange]

On Thu, Oct 09, 2008 at 06:50:15PM +0100, Daniel P. Berrange wrote:
> On Thu, Oct 09, 2008 at 11:22:38AM -0400, Ben Guthro wrote:
> > diff --git a/src/internal.h b/src/internal.h
> > index a3d48fa..67a3e5b 100644
> > --- a/src/internal.h
> > +++ b/src/internal.h
> > @@ -191,6 +191,18 @@ extern int debugFlag;
> >  #define VIR_IS_STORAGE_VOL(obj)		((obj) && (obj)->magic==VIR_STORAGE_VOL_MAGIC)
> >  #define VIR_IS_CONNECTED_STORAGE_VOL(obj)	(VIR_IS_STORAGE_VOL(obj) && VIR_IS_CONNECT((obj)->conn))
> >  
> > +/**
> > + * Domain Event Callbacks
> > + */
> > +struct _virDomainEventCallbackList {
> > +    virConnectPtr conn;
> > +    virConnectDomainEventCallback cb;
> > +    void *opaque;
> > +    struct _virDomainEventCallbackList *next;
> > +};
> > +
> > +typedef struct _virDomainEventCallbackList virDomainEventCallbackList;
> > +
> >  /*
> >   * arbitrary limitations
> >   */
> > @@ -237,6 +249,12 @@ struct _virConnect {
> >      virHashTablePtr storagePools;/* hash table for known storage pools */
> >      virHashTablePtr storageVols;/* hash table for known storage vols */
> >      int refs;                 /* reference count */
> > +
> > +    /* Domain Callbacks */
> > +    virDomainEventCallbackList *domainEventCallbacks;
> > +
> > +    /* link to next conn of this driver type */
> > +    struct _virConnect *next;
> >  };
> 
> This doesn't make any sense to me - you're making all the virConnect
> objects cross reference each other. Each object only needs to know
> about the callbacks registered to itself - it doesn't care about
> callbacks other virConnect objects have. I'd expect something 
> more like
> 
>    struct _virDomainEventCallback {
>       virConnectDomainEventCallback cb;
>       void *opaque;
>    };
>    typedef struct _virDomainEventCallback virDomainEventCallback;
>    typedef virDomainEventCallback *virDomainEventCallbackPtr;
> 
>    struct _virDomainEventCallbackList {
>      unsigned int ndomainCallbacks;
>      struct __virDomainEventCallbackPtr *domainCallbacks;
>    }
>    typedef struct _virDomainEventCallbackList virDomainEventCallbackList;
> 
> I used a explicit array here, because we're trying to kill off the linked
> lists, since it simplifies thread safety work, to be able to lock individual
> objects, and not worry about its peers pointing to it - only its parent.
> 
> The internal drivers would then have virDomainEventCallbackList intances
> as they need - eg, in the _xenUnifiedPrivate struct for Xen drivers, or
> in the 'struct qemud_driver' for QEMU, etc.

Correcting myself here - the _virDomainEventCallback struct does
actually need to keep a copy of the virConnectPtr object. Since
if we're storing it in the individual drivers, that's the only
link back to the connection. It can be treated opaquely though
in this instance - merely another pointer to be fed back into
the callback. Which reminds me, when registering the callback
it will be neccessary to increment the ref count on the virconnect
object to make sure it won't be de-allocated while an event 
calback is still present.

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 :|