On Thu, May 12, 2011 at 01:14:31PM -0400, Cole Robinson wrote:
> One functionality change here is that we no longer force enable the event
> timeout for every queued event, only enable it for the first event after
> the queue has been flushed. This is how other drivers have already done it,
> and I haven't encountered problems in practice.
>
> v3:
> Adjust for new virDomainEventStateNew argument
>
> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>
> ---
> .gnulib | 2 +-
> src/remote/remote_driver.c | 163 +++++++++++++++++---------------------------
> 2 files changed, 64 insertions(+), 101 deletions(-)
>
> diff --git a/.gnulib b/.gnulib
> index 64a5e38..a6676cc 160000
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit 64a5e38bced6c8f5117efbed95cdfd8ca133ed54
> +Subproject commit a6676cca6498ce67c5a3c8d7221b8d6c30b61dc4
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index c50ff25..1d64a63 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -184,15 +184,7 @@ struct private_data {
> unsigned int bufferLength;
> unsigned int bufferOffset;
>
> - /* The list of domain event callbacks */
> - virDomainEventCallbackListPtr callbackList;
> - /* The queue of domain events generated
> - during a call / response rpc */
> - virDomainEventQueuePtr domainEvents;
> - /* Timer for flushing domainEvents queue */
> - int eventFlushTimer;
> - /* Flag if we're in process of dispatching */
> - int domainEventDispatching;
> + virDomainEventStatePtr domainEventState;
>
> /* Self-pipe to wakeup threads waiting in poll() */
> int wakeupSendFD;
> @@ -264,6 +256,7 @@ static void make_nonnull_nwfilter (remote_nonnull_nwfilter *nwfilter_dst, virNWF
> static void make_nonnull_domain_snapshot (remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src);
> void remoteDomainEventFired(int watch, int fd, int event, void *data);
> void remoteDomainEventQueueFlush(int timer, void *opaque);
> +void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event);
> /*----------------------------------------------------------------------*/
>
> /* Helper functions for remoteOpen. */
> @@ -913,17 +906,6 @@ doRemoteOpen (virConnectPtr conn,
> }
> }
>
> - if(VIR_ALLOC(priv->callbackList)<0) {
> - virReportOOMError();
> - goto failed;
> - }
> -
> - if(VIR_ALLOC(priv->domainEvents)<0) {
> - virReportOOMError();
> - goto failed;
> - }
> -
> - VIR_DEBUG("Adding Handler for remote events");
> /* Set up a callback to listen on the socket data */
> if ((priv->watch = virEventAddHandle(priv->sock,
> VIR_EVENT_HANDLE_READABLE,
> @@ -931,18 +913,21 @@ doRemoteOpen (virConnectPtr conn,
> conn, NULL)) < 0) {
> VIR_DEBUG("virEventAddHandle failed: No addHandleImpl defined."
> " continuing without events.");
> - } else {
> + priv->watch = -1;
> + }
>
> - VIR_DEBUG("Adding Timeout for remote event queue flushing");
> - if ( (priv->eventFlushTimer = virEventAddTimeout(-1,
> - remoteDomainEventQueueFlush,
> - conn, NULL)) < 0) {
> - VIR_DEBUG("virEventAddTimeout failed: No addTimeoutImpl defined. "
> - "continuing without events.");
> - virEventRemoveHandle(priv->watch);
> - priv->watch = -1;
> - }
> + priv->domainEventState = virDomainEventStateNew(remoteDomainEventQueueFlush,
> + conn,
> + NULL,
> + false);
> + if (!priv->domainEventState) {
> + goto failed;
> + }
> + if (priv->domainEventState->timer < 0 && priv->watch != -1) {
> + virEventRemoveHandle(priv->watch);
> + priv->watch = -1;
> }
> +
> /* Successful. */
> retcode = VIR_DRV_OPEN_SUCCESS;
>
> @@ -1559,12 +1544,13 @@ verify_certificate (virConnectPtr conn ATTRIBUTE_UNUSED,
> static int
> doRemoteClose (virConnectPtr conn, struct private_data *priv)
> {
> - if (priv->eventFlushTimer >= 0) {
> - /* Remove timeout */
> - virEventRemoveTimeout(priv->eventFlushTimer);
> - /* Remove handle for remote events */
> + /* Remove timer before closing the connection, to avoid possible
> + * remoteDomainEventFired with a free'd connection */
> + if (priv->domainEventState->timer >= 0) {
> + virEventRemoveTimeout(priv->domainEventState->timer);
> virEventRemoveHandle(priv->watch);
> priv->watch = -1;
> + priv->domainEventState->timer = -1;
> }
>
> if (call (conn, priv, 0, REMOTE_PROC_CLOSE,
> @@ -1605,11 +1591,7 @@ retry:
> /* See comment for remoteType. */
> VIR_FREE(priv->type);
>
> - /* Free callback list */
> - virDomainEventCallbackListFree(priv->callbackList);
> -
> - /* Free queued events */
> - virDomainEventQueueFree(priv->domainEvents);
> + virDomainEventStateFree(priv->domainEventState);
>
> return 0;
> }
> @@ -3800,17 +3782,20 @@ static int remoteDomainEventRegister(virConnectPtr conn,
>
> remoteDriverLock(priv);
>
> - if (priv->eventFlushTimer < 0) {
> + if (priv->domainEventState->timer < 0) {
> remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support"));
> goto done;
> }
> - if (virDomainEventCallbackListAdd(conn, priv->callbackList,
> +
> + if (virDomainEventCallbackListAdd(conn, priv->domainEventState->callbacks,
> callback, opaque, freecb) < 0) {
> remoteError(VIR_ERR_RPC, "%s", _("adding cb to list"));
> goto done;
> }
>
> - if (virDomainEventCallbackListCountID(conn, priv->callbackList, VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 1) {
> + if (virDomainEventCallbackListCountID(conn,
> + priv->domainEventState->callbacks,
> + VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 1) {
> /* Tell the server when we are the first callback deregistering */
> if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_REGISTER,
> (xdrproc_t) xdr_void, (char *) NULL,
> @@ -3833,21 +3818,14 @@ static int remoteDomainEventDeregister(virConnectPtr conn,
>
> remoteDriverLock(priv);
>
> - if (priv->domainEventDispatching) {
> - if (virDomainEventCallbackListMarkDelete(conn, priv->callbackList,
> - callback) < 0) {
> - remoteError(VIR_ERR_RPC, "%s", _("marking cb for deletion"));
> - goto done;
> - }
> - } else {
> - if (virDomainEventCallbackListRemove(conn, priv->callbackList,
> - callback) < 0) {
> - remoteError(VIR_ERR_RPC, "%s", _("removing cb from list"));
> - goto done;
> - }
> - }
> + if (virDomainEventStateDeregister(conn,
> + priv->domainEventState,
> + callback) < 0)
> + goto done;
>
> - if (virDomainEventCallbackListCountID(conn, priv->callbackList, VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 0) {
> + if (virDomainEventCallbackListCountID(conn,
> + priv->domainEventState->callbacks,
> + VIR_DOMAIN_EVENT_ID_LIFECYCLE) == 0) {
> /* Tell the server when we are the last callback deregistering */
> if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER,
> (xdrproc_t) xdr_void, (char *) NULL,
> @@ -4723,12 +4701,13 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn,
>
> remoteDriverLock(priv);
>
> - if (priv->eventFlushTimer < 0) {
> + if (priv->domainEventState->timer < 0) {
> remoteError(VIR_ERR_NO_SUPPORT, "%s", _("no event support"));
> goto done;
> }
>
> - if ((callbackID = virDomainEventCallbackListAddID(conn, priv->callbackList,
> + if ((callbackID = virDomainEventCallbackListAddID(conn,
> + priv->domainEventState->callbacks,
> dom, eventID,
> callback, opaque, freecb)) < 0) {
> remoteError(VIR_ERR_RPC, "%s", _("adding cb to list"));
> @@ -4737,13 +4716,17 @@ static int remoteDomainEventRegisterAny(virConnectPtr conn,
>
> /* If this is the first callback for this eventID, we need to enable
> * events on the server */
> - if (virDomainEventCallbackListCountID(conn, priv->callbackList, eventID) == 1) {
> + if (virDomainEventCallbackListCountID(conn,
> + priv->domainEventState->callbacks,
> + eventID) == 1) {
> args.eventID = eventID;
>
> if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_REGISTER_ANY,
> (xdrproc_t) xdr_remote_domain_events_register_any_args, (char *) &args,
> (xdrproc_t) xdr_void, (char *)NULL) == -1) {
> - virDomainEventCallbackListRemoveID(conn, priv->callbackList, callbackID);
> + virDomainEventCallbackListRemoveID(conn,
> + priv->domainEventState->callbacks,
> + callbackID);
> goto done;
> }
> }
> @@ -4766,28 +4749,23 @@ static int remoteDomainEventDeregisterAny(virConnectPtr conn,
>
> remoteDriverLock(priv);
>
> - if ((eventID = virDomainEventCallbackListEventID(conn, priv->callbackList, callbackID)) < 0) {
> + if ((eventID = virDomainEventCallbackListEventID(conn,
> + priv->domainEventState->callbacks,
> + callbackID)) < 0) {
> remoteError(VIR_ERR_RPC, _("unable to find callback ID %d"), callbackID);
> goto done;
> }
>
> - if (priv->domainEventDispatching) {
> - if (virDomainEventCallbackListMarkDeleteID(conn, priv->callbackList,
> - callbackID) < 0) {
> - remoteError(VIR_ERR_RPC, "%s", _("marking cb for deletion"));
> - goto done;
> - }
> - } else {
> - if (virDomainEventCallbackListRemoveID(conn, priv->callbackList,
> - callbackID) < 0) {
> - remoteError(VIR_ERR_RPC, "%s", _("removing cb from list"));
> - goto done;
> - }
> - }
> + if (virDomainEventStateDeregisterAny(conn,
> + priv->domainEventState,
> + callbackID) < 0)
> + goto done;
>
> /* If that was the last callback for this eventID, we need to disable
> * events on the server */
> - if (virDomainEventCallbackListCountID(conn, priv->callbackList, eventID) == 0) {
> + if (virDomainEventCallbackListCountID(conn,
> + priv->domainEventState->callbacks,
> + eventID) == 0) {
> args.eventID = eventID;
>
> if (call (conn, priv, 0, REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER_ANY,
> @@ -5553,13 +5531,7 @@ processCallDispatchMessage(virConnectPtr conn, struct private_data *priv,
> if (!event)
> return -1;
>
> - if (virDomainEventQueuePush(priv->domainEvents,
> - event) < 0) {
> - VIR_DEBUG("Error adding event to queue");
> - virDomainEventFree(event);
> - }
> - virEventUpdateTimeout(priv->eventFlushTimer, 0);
> -
> + remoteDomainEventQueue(priv, event);
> return 0;
> }
>
> @@ -6230,31 +6202,22 @@ remoteDomainEventQueueFlush(int timer ATTRIBUTE_UNUSED, void *opaque)
> {
> virConnectPtr conn = opaque;
> struct private_data *priv = conn->privateData;
> - virDomainEventQueue tempQueue;
>
> - remoteDriverLock(priv);
>
> + remoteDriverLock(priv);
> VIR_DEBUG("Event queue flush %p", conn);
> - priv->domainEventDispatching = 1;
> -
> - /* Copy the queue, so we're reentrant safe */
> - tempQueue.count = priv->domainEvents->count;
> - tempQueue.events = priv->domainEvents->events;
> - priv->domainEvents->count = 0;
> - priv->domainEvents->events = NULL;
> -
> - virEventUpdateTimeout(priv->eventFlushTimer, -1);
> - virDomainEventQueueDispatch(&tempQueue, priv->callbackList,
> - remoteDomainEventDispatchFunc, priv);
> -
> - /* Purge any deleted callbacks */
> - virDomainEventCallbackListPurgeMarked(priv->callbackList);
> -
> - priv->domainEventDispatching = 0;
>
> + virDomainEventStateFlush(priv->domainEventState,
> + remoteDomainEventDispatchFunc,
> + priv);
> remoteDriverUnlock(priv);
> }
>
> +void
> +remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event)
> +{
> + virDomainEventStateQueue(priv->domainEventState, event);
> +}
>
> /* get_nonnull_domain and get_nonnull_network turn an on-wire
> * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list