[libvirt] [PATCH 0/8] events: Add helpers for driver dispatching
I noticed that there is quite a bit of code duplication among the drivers that support domain events. This patch series is an attempt to consolidate the shared logic. Patch #1 is the bugfix for https://bugzilla.redhat.com/show_bug.cgi?id=624252 which started me down this path. It can be applied independently of the refactoring if need be. Thanks, Cole Cole Robinson (8): remote: Don't lose track of events when callbacks are slow event-test: Simplify debug on/off domain_event: Add virDomainEventState structure domain_event: Add common domain event queue/flush helpers qemu: Use virDomainEventState helpers lxc: Use virDomainEventState helpers test: Use virDomainEventState helpers remote: Use virDomainEventState helpers examples/domain-events/events-python/event-test.py | 37 +++-- src/conf/domain_event.c | 163 +++++++++++++++++--- src/conf/domain_event.h | 64 ++++++-- src/libvirt_private.syms | 6 + src/lxc/lxc_conf.h | 6 +- src/lxc/lxc_driver.c | 76 +++------ src/qemu/qemu_conf.h | 6 +- src/qemu/qemu_driver.c | 80 +++------- src/remote/remote_driver.c | 162 ++++++++------------ src/test/test_driver.c | 104 ++++--------- 10 files changed, 363 insertions(+), 341 deletions(-) -- 1.7.3.3
After the remote driver runs an event callback, it unconditionally disables the loop timer, thinking it just flushed every queued event. This doesn't work correctly though if an event is queued while a callback is running. The events actually aren't being lost, it's just that the event loop didn't think there was anything that needed to be dispatched. So all those 'lost events' should actually get re-triggered if you manually kick the loop by generating a new event (like creating a new guest). The solution is to disable the dispatch timer _before_ we invoke any event callbacks. Events queued while a callback is running will properly reenable the timer. More info at https://bugzilla.redhat.com/show_bug.cgi?id=624252 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/remote/remote_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ee2de4a..ea119c6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -10557,9 +10557,9 @@ remoteDomainEventQueueFlush(int timer ATTRIBUTE_UNUSED, void *opaque) priv->domainEvents->count = 0; priv->domainEvents->events = NULL; + virEventUpdateTimeout(priv->eventFlushTimer, -1); virDomainEventQueueDispatch(&tempQueue, priv->callbackList, remoteDomainEventDispatchFunc, priv); - virEventUpdateTimeout(priv->eventFlushTimer, -1); /* Purge any deleted callbacks */ virDomainEventCallbackListPurgeMarked(priv->callbackList); -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
After the remote driver runs an event callback, it unconditionally disables the loop timer, thinking it just flushed every queued event. This doesn't work correctly though if an event is queued while a callback is running.
The events actually aren't being lost, it's just that the event loop didn't think there was anything that needed to be dispatched. So all those 'lost events' should actually get re-triggered if you manually kick the loop by generating a new event (like creating a new guest).
The solution is to disable the dispatch timer _before_ we invoke any event callbacks. Events queued while a callback is running will properly reenable the timer.
ACK. Matches the order in src/qemu/qemu_driver.c:qemuDomainEventFlush. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Make it easy to change debugging if being used by a client program. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- examples/domain-events/events-python/event-test.py | 37 ++++++++++--------- 1 files changed, 19 insertions(+), 18 deletions(-) diff --git a/examples/domain-events/events-python/event-test.py b/examples/domain-events/events-python/event-test.py index 7c5af68..903f934 100644 --- a/examples/domain-events/events-python/event-test.py +++ b/examples/domain-events/events-python/event-test.py @@ -15,6 +15,12 @@ import errno import time import threading +do_debug = False +def debug(msg): + global do_debug + if do_debug: + print msg + # # This general purpose event loop will support waiting for file handle # I/O and errors events, as well as scheduling repeatable timers with @@ -83,8 +89,7 @@ class virEventLoopPure: self.opaque[1]) - def __init__(self, debug=False): - self.debugOn = debug + def __init__(self): self.poll = select.poll() self.pipetrick = os.pipe() self.nextHandleID = 1 @@ -106,13 +111,9 @@ class virEventLoopPure: # with the event loop for input events. When we need to force # the main thread out of a poll() sleep, we simple write a # single byte of data to the other end of the pipe. - self.debug("Self pipe watch %d write %d" %(self.pipetrick[0], self.pipetrick[1])) + debug("Self pipe watch %d write %d" %(self.pipetrick[0], self.pipetrick[1])) self.poll.register(self.pipetrick[0], select.POLLIN) - def debug(self, msg): - if self.debugOn: - print msg - # Calculate when the next timeout is due to occurr, returning # the absolute timestamp for the next timeout, or 0 if there is @@ -166,7 +167,7 @@ class virEventLoopPure: def run_once(self): sleep = -1 next = self.next_timeout() - self.debug("Next timeout due at %d" % next) + debug("Next timeout due at %d" % next) if next > 0: now = int(time.time() * 1000) if now >= next: @@ -174,7 +175,7 @@ class virEventLoopPure: else: sleep = (next - now) / 1000.0 - self.debug("Poll with a sleep of %d" % sleep) + debug("Poll with a sleep of %d" % sleep) events = self.poll.poll(sleep) # Dispatch any file handle events that occurred @@ -188,7 +189,7 @@ class virEventLoopPure: h = self.get_handle_by_fd(fd) if h: - self.debug("Dispatch fd %d handle %d events %d" % (fd, h.get_id(), revents)) + debug("Dispatch fd %d handle %d events %d" % (fd, h.get_id(), revents)) h.dispatch(self.events_from_poll(revents)) now = int(time.time() * 1000) @@ -201,7 +202,7 @@ class virEventLoopPure: # Deduct 20ms, since schedular timeslice # means we could be ever so slightly early if now >= (want-20): - self.debug("Dispatch timer %d now %s want %s" % (t.get_id(), str(now), str(want))) + debug("Dispatch timer %d now %s want %s" % (t.get_id(), str(now), str(want))) t.set_last_fired(now) t.dispatch() @@ -230,7 +231,7 @@ class virEventLoopPure: self.poll.register(fd, self.events_to_poll(events)) self.interrupt() - self.debug("Add handle %d fd %d events %d" % (handleID, fd, events)) + debug("Add handle %d fd %d events %d" % (handleID, fd, events)) return handleID @@ -247,7 +248,7 @@ class virEventLoopPure: self.timers.append(h) self.interrupt() - self.debug("Add timer %d interval %d" % (timerID, interval)) + debug("Add timer %d interval %d" % (timerID, interval)) return timerID @@ -260,7 +261,7 @@ class virEventLoopPure: self.poll.register(h.get_fd(), self.events_to_poll(events)) self.interrupt() - self.debug("Update handle %d fd %d events %d" % (handleID, h.get_fd(), events)) + debug("Update handle %d fd %d events %d" % (handleID, h.get_fd(), events)) # Change the periodic frequency of the timer def update_timer(self, timerID, interval): @@ -269,7 +270,7 @@ class virEventLoopPure: h.set_interval(interval); self.interrupt() - self.debug("Update timer %d interval %d" % (timerID, interval)) + debug("Update timer %d interval %d" % (timerID, interval)) break # Stop monitoring for events on the file handle @@ -278,7 +279,7 @@ class virEventLoopPure: for h in self.handles: if h.get_id() == handleID: self.poll.unregister(h.get_fd()) - self.debug("Remove handle %d fd %d" % (handleID, h.get_fd())) + debug("Remove handle %d fd %d" % (handleID, h.get_fd())) else: handles.append(h) self.handles = handles @@ -290,7 +291,7 @@ class virEventLoopPure: for h in self.timers: if h.get_id() != timerID: timers.append(h) - self.debug("Remove timer %d" % timerID) + debug("Remove timer %d" % timerID) self.timers = timers self.interrupt() @@ -329,7 +330,7 @@ class virEventLoopPure: # This single global instance of the event loop wil be used for # monitoring libvirt events -eventLoop = virEventLoopPure(debug=False) +eventLoop = virEventLoopPure() # This keeps track of what thread is running the event loop, # (if it is run in a background thread) -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
Make it easy to change debugging if being used by a client program.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- examples/domain-events/events-python/event-test.py | 37 ++++++++++--------- 1 files changed, 19 insertions(+), 18 deletions(-)
I'm not that fluent in Python, but this is just example code (which I assume you've tested) and nothing jumped out as a red flag. So, if you're comfortable taking my review: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This structure will be used to unify lots of duplicated event handling code across the state drivers. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_event.c | 91 ++++++++++++++++++++++++++++++++++++---------- src/conf/domain_event.h | 47 +++++++++++++++++------- 2 files changed, 104 insertions(+), 34 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 5f086bd..4f0e4da 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -24,6 +24,7 @@ #include <config.h> #include "domain_event.h" +#include "event.h" #include "logging.h" #include "datatypes.h" #include "memory.h" @@ -505,6 +506,25 @@ void virDomainEventFree(virDomainEventPtr event) VIR_FREE(event); } +/** + * virDomainEventQueueFree: + * @queue: pointer to the queue + * + * Free the memory in the queue. We process this like a list here + */ +void +virDomainEventQueueFree(virDomainEventQueuePtr queue) +{ + int i; + if (!queue) + return; + + for (i = 0; i < queue->count ; i++) { + virDomainEventFree(queue->events[i]); + } + VIR_FREE(queue->events); + VIR_FREE(queue); +} virDomainEventQueuePtr virDomainEventQueueNew(void) { @@ -518,6 +538,57 @@ virDomainEventQueuePtr virDomainEventQueueNew(void) return ret; } +/** + * virDomainEventStateFree: + * @list: virDomainEventStatePtr to free + * + * Free a virDomainEventStatePtr and its members, and unregister the timer. + */ +void +virDomainEventStateFree(virDomainEventStatePtr state) +{ + virDomainEventCallbackListFree(state->callbacks); + virDomainEventQueueFree(state->queue); + + if (state->timer != -1) + virEventRemoveTimeout(state->timer); +} + +virDomainEventStatePtr +virDomainEventStateNew(virEventTimeoutCallback timeout_cb, + void *timeout_opaque, + virFreeCallback timeout_free) +{ + virDomainEventStatePtr state = NULL; + + if (VIR_ALLOC(state) < 0) { + virReportOOMError(); + goto error; + } + + if (VIR_ALLOC(state->callbacks) < 0) { + virReportOOMError(); + goto error; + } + + if (!(state->queue = virDomainEventQueueNew())) { + goto error; + } + + if ((state->timer = virEventAddTimeout(-1, + timeout_cb, + timeout_opaque, + timeout_free)) < 0) { + goto error; + } + + return state; + +error: + virDomainEventStateFree(state); + return NULL; +} + static virDomainEventPtr virDomainEventNewInternal(int eventID, int id, const char *name, @@ -776,26 +847,6 @@ virDomainEventPtr virDomainEventGraphicsNewFromObj(virDomainObjPtr obj, /** - * virDomainEventQueueFree: - * @queue: pointer to the queue - * - * Free the memory in the queue. We process this like a list here - */ -void -virDomainEventQueueFree(virDomainEventQueuePtr queue) -{ - int i; - if (!queue) - return; - - for (i = 0; i < queue->count ; i++) { - virDomainEventFree(queue->events[i]); - } - VIR_FREE(queue->events); - VIR_FREE(queue); -} - -/** * virDomainEventQueuePop: * @evtQueue: the queue of events * diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index e28293d..98a2870 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -25,6 +25,7 @@ #ifndef __DOMAIN_EVENT_H__ # define __DOMAIN_EVENT_H__ +# include "event.h" # include "domain_conf.h" typedef struct _virDomainEventCallback virDomainEventCallback; @@ -38,6 +39,33 @@ struct _virDomainEventCallbackList { typedef struct _virDomainEventCallbackList virDomainEventCallbackList; typedef virDomainEventCallbackList *virDomainEventCallbackListPtr; +/** + * Dispatching domain events that come in while + * in a call / response rpc + */ +typedef struct _virDomainEvent virDomainEvent; +typedef virDomainEvent *virDomainEventPtr; + +struct _virDomainEventQueue { + unsigned int count; + virDomainEventPtr *events; +}; +typedef struct _virDomainEventQueue virDomainEventQueue; +typedef virDomainEventQueue *virDomainEventQueuePtr; + +struct _virDomainEventState { + /* The list of domain event callbacks */ + virDomainEventCallbackListPtr callbacks; + /* The queue of domain events */ + virDomainEventQueuePtr queue; + /* Timer for flushing events queue */ + int timer; + /* Flag if we're in process of dispatching */ + int isDispatching; +}; +typedef struct _virDomainEventState virDomainEventState; +typedef virDomainEventState *virDomainEventStatePtr; + void virDomainEventCallbackListFree(virDomainEventCallbackListPtr list); int virDomainEventCallbackListAdd(virConnectPtr conn, @@ -91,20 +119,6 @@ int virDomainEventCallbackListEventID(virConnectPtr conn, int callbackID) ATTRIBUTE_NONNULL(1); -/** - * Dispatching domain events that come in while - * in a call / response rpc - */ -typedef struct _virDomainEvent virDomainEvent; -typedef virDomainEvent *virDomainEventPtr; - -struct _virDomainEventQueue { - unsigned int count; - virDomainEventPtr *events; -}; -typedef struct _virDomainEventQueue virDomainEventQueue; -typedef virDomainEventQueue *virDomainEventQueuePtr; - virDomainEventQueuePtr virDomainEventQueueNew(void); virDomainEventPtr virDomainEventNew(int id, const char *name, const unsigned char *uuid, int type, int detail); @@ -163,6 +177,11 @@ virDomainEventQueuePop(virDomainEventQueuePtr evtQueue); void virDomainEventFree(virDomainEventPtr event); void virDomainEventQueueFree(virDomainEventQueuePtr queue); +void virDomainEventStateFree(virDomainEventStatePtr state); +virDomainEventStatePtr +virDomainEventStateNew(virEventTimeoutCallback timeout_cb, + void *timeout_opaque, + virFreeCallback timeout_free); typedef void (*virDomainEventDispatchFunc)(virConnectPtr conn, virDomainEventPtr event, -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
This structure will be used to unify lots of duplicated event handling code across the state drivers.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_event.c | 91 ++++++++++++++++++++++++++++++++++++---------- src/conf/domain_event.h | 47 +++++++++++++++++------- 2 files changed, 104 insertions(+), 34 deletions(-)
Missing a tweak to the list of free-like functions in cfg.mk for virDomainEventStateFree.
+void +virDomainEventStateFree(virDomainEventStatePtr state) +{ + virDomainEventCallbackListFree(state->callbacks);
Missing an early exit for state==NULL, which is essential since virDomainEventStateNew's error: block can hit that scenario.
+ +struct _virDomainEventState { + /* The list of domain event callbacks */ + virDomainEventCallbackListPtr callbacks; + /* The queue of domain events */ + virDomainEventQueuePtr queue; + /* Timer for flushing events queue */ + int timer; + /* Flag if we're in process of dispatching */ + int isDispatching;
s/int/bool/
@@ -163,6 +177,11 @@ virDomainEventQueuePop(virDomainEventQueuePtr evtQueue);
void virDomainEventFree(virDomainEventPtr event); void virDomainEventQueueFree(virDomainEventQueuePtr queue); +void virDomainEventStateFree(virDomainEventStatePtr state); +virDomainEventStatePtr +virDomainEventStateNew(virEventTimeoutCallback timeout_cb, + void *timeout_opaque, + virFreeCallback timeout_free);
ATTRIBUTE_NONNULL(1) (the other two can reasonably be NULL, though). [Hmm, we probably are missing some other useful ATTRIBUTE_NONNULL markups throughout the event-related headers] -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The same code for queueing, flushing, and deregistering events exists in multiple drivers, which will soon use these common functions. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_event.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_event.h | 17 +++++++++++ 2 files changed, 90 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 4f0e4da..84f4709 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c @@ -579,7 +579,8 @@ virDomainEventStateNew(virEventTimeoutCallback timeout_cb, timeout_cb, timeout_opaque, timeout_free)) < 0) { - goto error; + DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. " + "continuing without events."); } return state; @@ -1048,3 +1049,74 @@ void virDomainEventQueueDispatch(virDomainEventQueuePtr queue, VIR_FREE(queue->events); queue->count = 0; } + +void +virDomainEventStateQueue(virDomainEventStatePtr state, + virDomainEventPtr event) +{ + if (state->timer < 0) { + virDomainEventFree(event); + return; + } + + if (virDomainEventQueuePush(state->queue, event) < 0) { + DEBUG0("Error adding event to queue"); + virDomainEventFree(event); + } + + if (state->queue->count == 1) + virEventUpdateTimeout(state->timer, 0); +} + +void +virDomainEventStateFlush(virDomainEventStatePtr state, + virDomainEventDispatchFunc dispatchFunc, + void *opaque) +{ + virDomainEventQueue tempQueue; + + state->isDispatching = 1; + + /* Copy the queue, so we're reentrant safe when dispatchFunc drops the + * driver lock */ + tempQueue.count = state->queue->count; + tempQueue.events = state->queue->events; + state->queue->count = 0; + state->queue->events = NULL; + + virEventUpdateTimeout(state->timer, -1); + virDomainEventQueueDispatch(&tempQueue, + state->callbacks, + dispatchFunc, + opaque); + + /* Purge any deleted callbacks */ + virDomainEventCallbackListPurgeMarked(state->callbacks); + + state->isDispatching = 0; +} + +int +virDomainEventStateDeregister(virConnectPtr conn, + virDomainEventStatePtr state, + virConnectDomainEventCallback callback) +{ + if (state->isDispatching) + return virDomainEventCallbackListMarkDelete(conn, + state->callbacks, callback); + else + return virDomainEventCallbackListRemove(conn, state->callbacks, callback); +} + +int +virDomainEventStateDeregisterAny(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID) +{ + if (state->isDispatching) + return virDomainEventCallbackListMarkDeleteID(conn, + state->callbacks, callbackID); + else + return virDomainEventCallbackListRemoveID(conn, + state->callbacks, callbackID); +} diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 98a2870..9893cf2 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -203,4 +203,21 @@ void virDomainEventQueueDispatch(virDomainEventQueuePtr queue, virDomainEventDispatchFunc dispatch, void *opaque); + +void +virDomainEventStateQueue(virDomainEventStatePtr state, + virDomainEventPtr event); +void +virDomainEventStateFlush(virDomainEventStatePtr state, + virDomainEventDispatchFunc dispatchFunc, + void *opaque); +int +virDomainEventStateDeregister(virConnectPtr conn, + virDomainEventStatePtr state, + virConnectDomainEventCallback callback); +int +virDomainEventStateDeregisterAny(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID); + #endif -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
The same code for queueing, flushing, and deregistering events exists in multiple drivers, which will soon use these common functions.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_event.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_event.h | 17 +++++++++++ 2 files changed, 90 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c index 4f0e4da..84f4709 100644 --- a/src/conf/domain_event.c +++ b/src/conf/domain_event.c +void +virDomainEventStateFlush(virDomainEventStatePtr state, + virDomainEventDispatchFunc dispatchFunc, + void *opaque) +{ + virDomainEventQueue tempQueue; + + state->isDispatching = 1;
s/1/true/, if you changed the type of isDispatching per my comment on 3/8.
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h index 98a2870..9893cf2 100644 --- a/src/conf/domain_event.h +++ b/src/conf/domain_event.h @@ -203,4 +203,21 @@ void virDomainEventQueueDispatch(virDomainEventQueuePtr queue, virDomainEventDispatchFunc dispatch, void *opaque);
+ +void +virDomainEventStateQueue(virDomainEventStatePtr state, + virDomainEventPtr event);
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+void +virDomainEventStateFlush(virDomainEventStatePtr state, + virDomainEventDispatchFunc dispatchFunc, + void *opaque);
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
+int +virDomainEventStateDeregister(virConnectPtr conn, + virDomainEventStatePtr state, + virConnectDomainEventCallback callback);
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+int +virDomainEventStateDeregisterAny(virConnectPtr conn, + virDomainEventStatePtr state, + int callbackID);
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 6 +++ src/qemu/qemu_conf.h | 6 +--- src/qemu/qemu_driver.c | 80 +++++++++++++++------------------------------- 3 files changed, 33 insertions(+), 59 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 19e581c..2fcecc8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -349,6 +349,12 @@ virDomainEventRTCChangeNewFromDom; virDomainEventRTCChangeNewFromObj; virDomainEventRebootNewFromDom; virDomainEventRebootNewFromObj; +virDomainEventStateDeregister; +virDomainEventStateDeregisterAny; +virDomainEventStateFlush; +virDomainEventStateQueue; +virDomainEventStateFree; +virDomainEventStateNew; virDomainEventWatchdogNewFromDom; virDomainEventWatchdogNewFromObj; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 83ddedd..0733822 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; virSecurityDriverPtr securityDriver; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e915705..8ceae48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1198,15 +1198,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; + } /* Allocate bitmap for vnc port reservation */ if ((qemu_driver->reservedVNCPorts = @@ -1567,12 +1569,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->cgroupDeviceACL); } - /* Free domain callback list */ - virDomainEventCallbackListFree(qemu_driver->domainEventCallbacks); - virDomainEventQueueFree(qemu_driver->domainEventQueue); - - if (qemu_driver->domainEventTimer != -1) - virEventRemoveTimeout(qemu_driver->domainEventTimer); + virDomainEventStateFree(qemu_driver->domainEventState); if (qemu_driver->brctl) brShutdown(qemu_driver->brctl); @@ -3206,7 +3203,8 @@ static int qemudClose(virConnectPtr conn) { /* Get rid of callbacks registered for this conn */ qemuDriverLock(driver); - virDomainEventCallbackListRemoveConn(conn, driver->domainEventCallbacks); + virDomainEventCallbackListRemoveConn(conn, + driver->domainEventState->callbacks); qemuDriverUnlock(driver); conn->privateData = NULL; @@ -7815,7 +7813,8 @@ qemuDomainEventRegister(virConnectPtr conn, int ret; qemuDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, driver->domainEventCallbacks, + ret = virDomainEventCallbackListAdd(conn, + driver->domainEventState->callbacks, callback, opaque, freecb); qemuDriverUnlock(driver); @@ -7831,12 +7830,9 @@ qemuDomainEventDeregister(virConnectPtr conn, int ret; qemuDriverLock(driver); - if (driver->domainEventDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, driver->domainEventCallbacks, - callback); - else - ret = virDomainEventCallbackListRemove(conn, driver->domainEventCallbacks, - callback); + ret = virDomainEventStateDeregister(conn, + driver->domainEventState, + callback); qemuDriverUnlock(driver); return ret; @@ -7856,7 +7852,7 @@ qemuDomainEventRegisterAny(virConnectPtr conn, qemuDriverLock(driver); ret = virDomainEventCallbackListAddID(conn, - driver->domainEventCallbacks, + driver->domainEventState->callbacks, dom, eventID, callback, opaque, freecb); qemuDriverUnlock(driver); @@ -7873,12 +7869,9 @@ qemuDomainEventDeregisterAny(virConnectPtr conn, int ret; qemuDriverLock(driver); - if (driver->domainEventDispatching) - ret = virDomainEventCallbackListMarkDeleteID(conn, driver->domainEventCallbacks, - callbackID); - else - ret = virDomainEventCallbackListRemoveID(conn, driver->domainEventCallbacks, - callbackID); + ret = virDomainEventStateDeregisterAny(conn, + driver->domainEventState, + callbackID); qemuDriverUnlock(driver); return ret; @@ -7902,28 +7895,11 @@ static void qemuDomainEventDispatchFunc(virConnectPtr conn, static void qemuDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) { struct qemud_driver *driver = opaque; - virDomainEventQueue tempQueue; qemuDriverLock(driver); - - driver->domainEventDispatching = 1; - - /* Copy the queue, so we're reentrant safe */ - tempQueue.count = driver->domainEventQueue->count; - tempQueue.events = driver->domainEventQueue->events; - driver->domainEventQueue->count = 0; - driver->domainEventQueue->events = NULL; - - virEventUpdateTimeout(driver->domainEventTimer, -1); - virDomainEventQueueDispatch(&tempQueue, - driver->domainEventCallbacks, - qemuDomainEventDispatchFunc, - driver); - - /* Purge any deleted callbacks */ - virDomainEventCallbackListPurgeMarked(driver->domainEventCallbacks); - - driver->domainEventDispatching = 0; + virDomainEventStateFlush(driver->domainEventState, + qemuDomainEventDispatchFunc, + driver); qemuDriverUnlock(driver); } @@ -7932,11 +7908,7 @@ static void qemuDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) static void qemuDomainEventQueue(struct qemud_driver *driver, virDomainEventPtr event) { - if (virDomainEventQueuePush(driver->domainEventQueue, - event) < 0) - virDomainEventFree(event); - if (qemu_driver->domainEventQueue->count == 1) - virEventUpdateTimeout(driver->domainEventTimer, 0); + virDomainEventStateQueue(driver->domainEventState, event); } /* Migration support. */ -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 6 +++ src/qemu/qemu_conf.h | 6 +--- src/qemu/qemu_driver.c | 80 +++++++++++++++------------------------------- 3 files changed, 33 insertions(+), 59 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 19e581c..2fcecc8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -349,6 +349,12 @@ virDomainEventRTCChangeNewFromDom; virDomainEventRTCChangeNewFromObj; virDomainEventRebootNewFromDom; virDomainEventRebootNewFromObj; +virDomainEventStateDeregister; +virDomainEventStateDeregisterAny; +virDomainEventStateFlush; +virDomainEventStateQueue; +virDomainEventStateFree; +virDomainEventStateNew;
Sort these lines :) ACK; looks like a nice refactoring, once the earlier patches have nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/10/2011 10:38 AM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 6 +++ src/qemu/qemu_conf.h | 6 +--- src/qemu/qemu_driver.c | 80 +++++++++++++++------------------------------- 3 files changed, 33 insertions(+), 59 deletions(-)
Actually - how about shuffling the changes to src/libvirt_private.syms into patch 4/8; that way, if someone independently applies 6/8 without 5/8, things still work. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_conf.h | 6 +--- src/lxc/lxc_driver.c | 76 ++++++++++++++++---------------------------------- 2 files changed, 25 insertions(+), 57 deletions(-) diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index f820d6d..308a3dd 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -55,11 +55,7 @@ struct __lxc_driver { int log_libvirtd; int have_netns; - /* An array of callbacks */ - virDomainEventCallbackListPtr domainEventCallbacks; - virDomainEventQueuePtr domainEventQueue; - int domainEventTimer; - int domainEventDispatching; + virDomainEventStatePtr domainEventState; }; int lxcLoadDriverConfig(lxc_driver_t *driver); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index eb58086..8717d08 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -158,7 +158,8 @@ static int lxcClose(virConnectPtr conn) lxc_driver_t *driver = conn->privateData; lxcDriverLock(driver); - virDomainEventCallbackListRemoveConn(conn, driver->domainEventCallbacks); + virDomainEventCallbackListRemoveConn(conn, + driver->domainEventState->callbacks); lxcDriverUnlock(driver); conn->privateData = NULL; @@ -1772,7 +1773,8 @@ lxcDomainEventRegister(virConnectPtr conn, int ret; lxcDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, driver->domainEventCallbacks, + ret = virDomainEventCallbackListAdd(conn, + driver->domainEventState->callbacks, callback, opaque, freecb); lxcDriverUnlock(driver); @@ -1788,12 +1790,9 @@ lxcDomainEventDeregister(virConnectPtr conn, int ret; lxcDriverLock(driver); - if (driver->domainEventDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, driver->domainEventCallbacks, - callback); - else - ret = virDomainEventCallbackListRemove(conn, driver->domainEventCallbacks, - callback); + ret = virDomainEventStateDeregister(conn, + driver->domainEventState, + callback); lxcDriverUnlock(driver); return ret; @@ -1813,7 +1812,7 @@ lxcDomainEventRegisterAny(virConnectPtr conn, lxcDriverLock(driver); ret = virDomainEventCallbackListAddID(conn, - driver->domainEventCallbacks, + driver->domainEventState->callbacks, dom, eventID, callback, opaque, freecb); lxcDriverUnlock(driver); @@ -1830,12 +1829,9 @@ lxcDomainEventDeregisterAny(virConnectPtr conn, int ret; lxcDriverLock(driver); - if (driver->domainEventDispatching) - ret = virDomainEventCallbackListMarkDeleteID(conn, driver->domainEventCallbacks, - callbackID); - else - ret = virDomainEventCallbackListRemoveID(conn, driver->domainEventCallbacks, - callbackID); + ret = virDomainEventStateDeregisterAny(conn, + driver->domainEventState, + callbackID); lxcDriverUnlock(driver); return ret; @@ -1860,28 +1856,11 @@ static void lxcDomainEventDispatchFunc(virConnectPtr conn, static void lxcDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) { lxc_driver_t *driver = opaque; - virDomainEventQueue tempQueue; lxcDriverLock(driver); - - driver->domainEventDispatching = 1; - - /* Copy the queue, so we're reentrant safe */ - tempQueue.count = driver->domainEventQueue->count; - tempQueue.events = driver->domainEventQueue->events; - driver->domainEventQueue->count = 0; - driver->domainEventQueue->events = NULL; - - virEventUpdateTimeout(driver->domainEventTimer, -1); - virDomainEventQueueDispatch(&tempQueue, - driver->domainEventCallbacks, - lxcDomainEventDispatchFunc, - driver); - - /* Purge any deleted callbacks */ - virDomainEventCallbackListPurgeMarked(driver->domainEventCallbacks); - - driver->domainEventDispatching = 0; + virDomainEventStateFlush(driver->domainEventState, + lxcDomainEventDispatchFunc, + driver); lxcDriverUnlock(driver); } @@ -1890,11 +1869,7 @@ static void lxcDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) static void lxcDomainEventQueue(lxc_driver_t *driver, virDomainEventPtr event) { - if (virDomainEventQueuePush(driver->domainEventQueue, - event) < 0) - virDomainEventFree(event); - if (lxc_driver->domainEventQueue->count == 1) - virEventUpdateTimeout(driver->domainEventTimer, 0); + virDomainEventStateQueue(driver->domainEventState, event); } /** @@ -2109,14 +2084,16 @@ static int lxcStartup(int privileged) if (virDomainObjListInit(&lxc_driver->domains) < 0) goto cleanup; - if (VIR_ALLOC(lxc_driver->domainEventCallbacks) < 0) + lxc_driver->domainEventState = virDomainEventStateNew(lxcDomainEventFlush, + lxc_driver, + NULL); + if (!lxc_driver->domainEventState) goto cleanup; - if (!(lxc_driver->domainEventQueue = virDomainEventQueueNew())) - goto cleanup; - - if ((lxc_driver->domainEventTimer = - virEventAddTimeout(-1, lxcDomainEventFlush, lxc_driver, NULL)) < 0) + if (!lxc_driver->domainEventState->timer < 0) { + lxcError(VIR_ERR_INTERNAL_ERROR, "%s", + _("could not initialize domain event timer")); goto cleanup; + } lxc_driver->log_libvirtd = 0; /* by default log to container logfile */ lxc_driver->have_netns = lxcCheckNetNsSupport(); @@ -2204,12 +2181,7 @@ static int lxcShutdown(void) lxcDriverLock(lxc_driver); virDomainObjListDeinit(&lxc_driver->domains); - - virDomainEventCallbackListFree(lxc_driver->domainEventCallbacks); - virDomainEventQueueFree(lxc_driver->domainEventQueue); - - if (lxc_driver->domainEventTimer != -1) - virEventRemoveTimeout(lxc_driver->domainEventTimer); + virDomainEventStateFree(lxc_driver->domainEventState); virCapabilitiesFree(lxc_driver->caps); VIR_FREE(lxc_driver->configDir); -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_conf.h | 6 +--- src/lxc/lxc_driver.c | 76 ++++++++++++++++---------------------------------- 2 files changed, 25 insertions(+), 57 deletions(-)
ACK -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 104 ++++++++++++++--------------------------------- 1 files changed, 31 insertions(+), 73 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ddff160..87350e8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -90,12 +90,7 @@ struct _testConn { int numCells; testCell cells[MAX_CELLS]; - - /* An array of callbacks */ - virDomainEventCallbackListPtr domainEventCallbacks; - virDomainEventQueuePtr domainEventQueue; - int domainEventTimer; - int domainEventDispatching; + virDomainEventStatePtr domainEventState; }; typedef struct _testConn testConn; typedef struct _testConn *testConnPtr; @@ -1124,6 +1119,7 @@ static virDrvOpenStatus testOpen(virConnectPtr conn, int flags ATTRIBUTE_UNUSED) { int ret; + testConnPtr privconn; if (!conn->uri) return VIR_DRV_OPEN_DECLINED; @@ -1150,26 +1146,24 @@ static virDrvOpenStatus testOpen(virConnectPtr conn, ret = testOpenFromFile(conn, conn->uri->path); - if (ret == VIR_DRV_OPEN_SUCCESS) { - testConnPtr privconn = conn->privateData; - testDriverLock(privconn); - /* Init callback list */ - if (VIR_ALLOC(privconn->domainEventCallbacks) < 0 || - !(privconn->domainEventQueue = virDomainEventQueueNew())) { - virReportOOMError(); - testDriverUnlock(privconn); - testClose(conn); - return VIR_DRV_OPEN_ERROR; - } + if (ret != VIR_DRV_OPEN_SUCCESS) + return ret; + + privconn = conn->privateData; + testDriverLock(privconn); - if ((privconn->domainEventTimer = - virEventAddTimeout(-1, testDomainEventFlush, privconn, NULL)) < 0) - DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. " - "continuing without events."); + privconn->domainEventState = virDomainEventStateNew(testDomainEventFlush, + privconn, + NULL); + if (!privconn->domainEventState) { testDriverUnlock(privconn); + testClose(conn); + return VIR_DRV_OPEN_ERROR; } - return (ret); + testDriverUnlock(privconn); + + return VIR_DRV_OPEN_SUCCESS; } static int testClose(virConnectPtr conn) @@ -1182,12 +1176,7 @@ static int testClose(virConnectPtr conn) virNetworkObjListFree(&privconn->networks); virInterfaceObjListFree(&privconn->ifaces); virStoragePoolObjListFree(&privconn->pools); - - virDomainEventCallbackListFree(privconn->domainEventCallbacks); - virDomainEventQueueFree(privconn->domainEventQueue); - - if (privconn->domainEventTimer != -1) - virEventRemoveTimeout(privconn->domainEventTimer); + virDomainEventStateFree(privconn->domainEventState); testDriverUnlock(privconn); virMutexDestroy(&privconn->lock); @@ -5177,7 +5166,8 @@ testDomainEventRegister(virConnectPtr conn, int ret; testDriverLock(driver); - ret = virDomainEventCallbackListAdd(conn, driver->domainEventCallbacks, + ret = virDomainEventCallbackListAdd(conn, + driver->domainEventState->callbacks, callback, opaque, freecb); testDriverUnlock(driver); @@ -5193,12 +5183,9 @@ testDomainEventDeregister(virConnectPtr conn, int ret; testDriverLock(driver); - if (driver->domainEventDispatching) - ret = virDomainEventCallbackListMarkDelete(conn, driver->domainEventCallbacks, - callback); - else - ret = virDomainEventCallbackListRemove(conn, driver->domainEventCallbacks, - callback); + ret = virDomainEventStateDeregister(conn, + driver->domainEventState, + callback); testDriverUnlock(driver); return ret; @@ -5217,7 +5204,8 @@ testDomainEventRegisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - ret = virDomainEventCallbackListAddID(conn, driver->domainEventCallbacks, + ret = virDomainEventCallbackListAddID(conn, + driver->domainEventState->callbacks, dom, eventID, callback, opaque, freecb); testDriverUnlock(driver); @@ -5233,12 +5221,9 @@ testDomainEventDeregisterAny(virConnectPtr conn, int ret; testDriverLock(driver); - if (driver->domainEventDispatching) - ret = virDomainEventCallbackListMarkDeleteID(conn, driver->domainEventCallbacks, - callbackID); - else - ret = virDomainEventCallbackListRemoveID(conn, driver->domainEventCallbacks, - callbackID); + ret = virDomainEventStateDeregisterAny(conn, + driver->domainEventState, + callbackID); testDriverUnlock(driver); return ret; @@ -5262,28 +5247,11 @@ static void testDomainEventDispatchFunc(virConnectPtr conn, static void testDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) { testConnPtr driver = opaque; - virDomainEventQueue tempQueue; testDriverLock(driver); - - driver->domainEventDispatching = 1; - - /* Copy the queue, so we're reentrant safe */ - tempQueue.count = driver->domainEventQueue->count; - tempQueue.events = driver->domainEventQueue->events; - driver->domainEventQueue->count = 0; - driver->domainEventQueue->events = NULL; - - virEventUpdateTimeout(driver->domainEventTimer, -1); - virDomainEventQueueDispatch(&tempQueue, - driver->domainEventCallbacks, - testDomainEventDispatchFunc, - driver); - - /* Purge any deleted callbacks */ - virDomainEventCallbackListPurgeMarked(driver->domainEventCallbacks); - - driver->domainEventDispatching = 0; + virDomainEventStateFlush(driver->domainEventState, + testDomainEventDispatchFunc, + driver); testDriverUnlock(driver); } @@ -5292,17 +5260,7 @@ static void testDomainEventFlush(int timer ATTRIBUTE_UNUSED, void *opaque) static void testDomainEventQueue(testConnPtr driver, virDomainEventPtr event) { - if (driver->domainEventTimer < 0) { - virDomainEventFree(event); - return; - } - - if (virDomainEventQueuePush(driver->domainEventQueue, - event) < 0) - virDomainEventFree(event); - - if (driver->domainEventQueue->count == 1) - virEventUpdateTimeout(driver->domainEventTimer, 0); + virDomainEventStateQueue(driver->domainEventState, event); } static virDrvOpenStatus testSecretOpen(virConnectPtr conn, -- 1.7.3.3
On 01/10/2011 10:38 AM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/test/test_driver.c | 104 ++++++++++++++--------------------------------- 1 files changed, 31 insertions(+), 73 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 it haven't encountered problems in practice. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/remote/remote_driver.c | 162 +++++++++++++++++--------------------------- 1 files changed, 62 insertions(+), 100 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index ea119c6..56f99ff 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -182,15 +182,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; @@ -262,6 +254,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. */ @@ -897,17 +890,8 @@ doRemoteOpen (virConnectPtr conn, } } - if(VIR_ALLOC(priv->callbackList)<0) { - virReportOOMError(); - goto failed; - } - - if(VIR_ALLOC(priv->domainEvents)<0) { - virReportOOMError(); - goto failed; - } - DEBUG0("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, @@ -915,18 +899,20 @@ doRemoteOpen (virConnectPtr conn, conn, NULL)) < 0) { DEBUG0("virEventAddHandle failed: No addHandleImpl defined." " continuing without events."); - } else { + priv->watch = -1; + } - DEBUG0("Adding Timeout for remote event queue flushing"); - if ( (priv->eventFlushTimer = virEventAddTimeout(-1, - remoteDomainEventQueueFlush, - conn, NULL)) < 0) { - DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. " - "continuing without events."); - virEventRemoveHandle(priv->watch); - priv->watch = -1; - } + priv->domainEventState = virDomainEventStateNew(remoteDomainEventQueueFlush, + conn, + NULL); + 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; @@ -1440,12 +1426,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, @@ -1486,11 +1473,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; } @@ -7506,17 +7489,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, @@ -7539,21 +7525,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, @@ -9159,12 +9138,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")); @@ -9173,13 +9153,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; } } @@ -9202,28 +9186,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, @@ -9901,13 +9880,7 @@ processCallDispatchMessage(virConnectPtr conn, struct private_data *priv, if (!event) return -1; - if (virDomainEventQueuePush(priv->domainEvents, - event) < 0) { - DEBUG0("Error adding event to queue"); - virDomainEventFree(event); - } - virEventUpdateTimeout(priv->eventFlushTimer, 0); - + remoteDomainEventQueue(priv, event); return 0; } @@ -10545,30 +10518,19 @@ remoteDomainEventQueueFlush(int timer ATTRIBUTE_UNUSED, void *opaque) { virConnectPtr conn = opaque; struct private_data *priv = conn->privateData; - virDomainEventQueue tempQueue; remoteDriverLock(priv); - - 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. -- 1.7.3.3
On 01/10/2011 10:38 AM, 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 it haven't encountered problems in practice.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/remote/remote_driver.c | 162 +++++++++++++++++--------------------------- 1 files changed, 62 insertions(+), 100 deletions(-)
The functionality change seems reasonable, and I didn't spot any problems in this patch. ACK. The comments earlier in the series didn't seem difficult to resolve, but were probably enough to warrant posting a v2 once you've addressed them. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/10/2011 01:45 PM, Eric Blake wrote:
On 01/10/2011 10:38 AM, 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 it haven't encountered problems in practice.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/remote/remote_driver.c | 162 +++++++++++++++++--------------------------- 1 files changed, 62 insertions(+), 100 deletions(-)
The functionality change seems reasonable, and I didn't spot any problems in this patch. ACK.
The comments earlier in the series didn't seem difficult to resolve, but were probably enough to warrant posting a v2 once you've addressed them.
Thanks for the review, I pushed the first 2 patches and will respin the rest. - Cole
participants (2)
-
Cole Robinson -
Eric Blake