Re: [libvirt] [PATCH 3/6] qemu: Use virDomainEventState helpers

On Mon, Jan 10, 2011 at 03:28:57PM -0500, Cole Robinson wrote: > v2: > Drop libvirt_private.syms changes > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > --- > src/qemu/qemu_conf.h | 6 +--- > src/qemu/qemu_driver.c | 80 +++++++++++++++-------------------------------- > 2 files changed, 27 insertions(+), 59 deletions(-) > > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index 5a5748b..2d878b5 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -107,11 +107,7 @@ struct qemud_driver { > > virCapsPtr caps; > > - /* An array of callbacks */ > - virDomainEventCallbackListPtr domainEventCallbacks; > - virDomainEventQueuePtr domainEventQueue; > - int domainEventTimer; > - int domainEventDispatching; > + virDomainEventStatePtr domainEventState; > > char *securityDriverName; > virSecurityManagerPtr securityManager; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 9eb9cd5..5acf1d9 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -1185,15 +1185,17 @@ qemudStartup(int privileged) { > if (virDomainObjListInit(&qemu_driver->domains) < 0) > goto out_of_memory; > > - /* Init callback list */ > - if (VIR_ALLOC(qemu_driver->domainEventCallbacks) < 0) > - goto out_of_memory; > - if (!(qemu_driver->domainEventQueue = virDomainEventQueueNew())) > - goto out_of_memory; > - > - if ((qemu_driver->domainEventTimer = > - virEventAddTimeout(-1, qemuDomainEventFlush, qemu_driver, NULL)) < 0) > + /* Init domain events */ > + qemu_driver->domainEventState = virDomainEventStateNew(qemuDomainEventFlush, > + qemu_driver, > + NULL); > + if (!qemu_driver->domainEventState) > goto error; > + if (!qemu_driver->domainEventState->timer < 0) { > + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("could not initialize domain event timer")); > + goto error; > + } I'm curious about this ->timer < 0 check here. I'd kind of expect the 'domainEventState' struct to be opaque, and only accessible to the helper APIs you added as virDomainEventStateXXX. Could the virDomainEventStateNew() function simply return NULL if it was unable to initilize the timer, or is there a need for this to be treated as a non-fatal scenario ?

If it needs to be treated as non-fatal, could we pass in a bool flag to virDomainEventStateNew() 'bool requireTimer' so that it can enforce this itself rather than making the callers check