[libvirt] PATCH: Pass a callback for freeing opaque data when registering handle/timer events

When registering for a file descriptor event, or timer events, the event callback has an associated 'void *opaque' data blob. When removing a registered event, the removal may be done asynchronously to allow safe removal from within a callback. This means that it is not safe for the application to assume they can free the 'void *opaque' data immediately after calling virEventRemoveHandle/Timer. So, we extend the AddHandle/Timer method to allow a 2nd callback to be provided. This callback is used to free the 'void *opaque' data at the appropriate (safe) point in time. examples/domain-events/events-c/event-test.c | 24 ++++++++++-- include/libvirt/libvirt.h | 52 ++++++++++++++++++++++++--- include/libvirt/libvirt.h.in | 52 ++++++++++++++++++++++++--- python/libvir.c | 25 +++++++++--- python/libvirt_wrap.h | 2 + python/types.c | 32 ++++++++++++++++ qemud/event.c | 19 ++++++++- qemud/event.h | 9 +++- qemud/mdns.c | 22 +++++++++-- qemud/qemud.c | 9 ++++ src/event.c | 14 +++++-- src/event.h | 9 +++- src/lxc_driver.c | 1 src/qemu_driver.c | 2 + src/remote_internal.c | 2 + 15 files changed, 240 insertions(+), 34 deletions(-) Daniel diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -22,6 +22,7 @@ int h_fd = 0; int h_fd = 0; virEventHandleType h_event = 0; virEventHandleCallback h_cb = NULL; +virEventHandleFreeFunc h_ff = NULL; void *h_opaque = NULL; /* timeout globals */ @@ -29,6 +30,7 @@ int t_active = 0; int t_active = 0; int t_timeout = -1; virEventTimeoutCallback t_cb = NULL; +virEventTimeoutFreeFunc t_ff = NULL; void *t_opaque = NULL; @@ -39,11 +41,15 @@ int myDomainEventCallback2 (virConnectPt int myDomainEventCallback2 (virConnectPtr conn, virDomainPtr dom, int event, int detail, void *opaque); int myEventAddHandleFunc (int fd, int event, - virEventHandleCallback cb, void *opaque); + virEventHandleCallback cb, + virEventHandleFreeFunc ff, + void *opaque); void myEventUpdateHandleFunc(int watch, int event); int myEventRemoveHandleFunc(int watch); -int myEventAddTimeoutFunc(int timeout, virEventTimeoutCallback cb, +int myEventAddTimeoutFunc(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, void *opaque); void myEventUpdateTimeoutFunc(int timer, int timout); int myEventRemoveTimeoutFunc(int timer); @@ -199,12 +205,15 @@ virEventHandleType myPollEventToEventHan } int myEventAddHandleFunc(int fd, int event, - virEventHandleCallback cb, void *opaque) + virEventHandleCallback cb, + virEventHandleFreeFunc ff, + void *opaque) { DEBUG("Add handle %d %d %p %p", fd, event, cb, opaque); h_fd = fd; h_event = myEventHandleTypeToPollEvent(event); h_cb = cb; + h_ff = ff; h_opaque = opaque; return 0; } @@ -220,16 +229,21 @@ int myEventRemoveHandleFunc(int fd) { DEBUG("Removed Handle %d", fd); h_fd = 0; + if (h_ff) + (h_ff)(h_opaque); return 0; } -int myEventAddTimeoutFunc(int timeout, virEventTimeoutCallback cb, +int myEventAddTimeoutFunc(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, void *opaque) { DEBUG("Adding Timeout %d %p %p", timeout, cb, opaque); t_active = 1; t_timeout = timeout; t_cb = cb; + t_ff = ff; t_opaque = opaque; return 0; } @@ -244,6 +258,8 @@ int myEventRemoveTimeoutFunc(int timer) { DEBUG("Timeout removed %d", timer); t_active = 0; + if (t_ff) + (t_ff)(t_opaque); return 0; } diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -1132,21 +1132,39 @@ typedef void (*virEventHandleCallback)(i typedef void (*virEventHandleCallback)(int watch, int fd, int events, void *opaque); /** + * virEventHandleFreeFunc: + * + * @opaque: user data to be free'd + * + * Callback invoked when memory associated with a file handle + * watch is being freed. Will be passed a pointer to the application's + * opaque data blob. + */ +typedef void (*virEventHandleFreeFunc)(void *opaque); + +/** * virEventAddHandleFunc: * @fd: file descriptor to listen on * @event: bitset of events on which to fire the callback - * @cb: the callback to be called + * @cb: the callback to be called when an event occurrs + * @ff: the callback invoked to free opaque data blob * @opaque: user data to pass to the callback * * Part of the EventImpl, this callback Adds a file handle callback to * listen for specific events. The same file handle can be registered * multiple times provided the requested event sets are non-overlapping * + * If the opaque user data requires free'ing when the handle + * is unregistered, then a 2nd callback can be supplied for + * this purpose. + * * Returns a handle watch number to be used for updating * and unregistering for events */ typedef int (*virEventAddHandleFunc)(int fd, int event, - virEventHandleCallback cb, void *opaque); + virEventHandleCallback cb, + virEventHandleFreeFunc ff, + void *opaque); /** * virEventUpdateHandleFunc: @@ -1163,7 +1181,11 @@ typedef void (*virEventUpdateHandleFunc) * @watch: file descriptor watch to stop listening on * * Part of the EventImpl, this user-provided callback is notified when - * an fd is no longer being listened on + * an fd is no longer being listened on. + * + * If a virEventHandleFreeFunc was supplied when the handle was + * registered, it will be invoked some time during, or after this + * function call, when it is safe to release the user data. */ typedef int (*virEventRemoveHandleFunc)(int watch); @@ -1178,17 +1200,35 @@ typedef void (*virEventTimeoutCallback)( typedef void (*virEventTimeoutCallback)(int timer, void *opaque); /** + * virEventTimeoutFreeFunc: + * + * @opaque: user data to be free'd + * + * Callback invoked when memory associated with a timer + * is being freed. Will be passed a pointer to the application's + * opaque data blob. + */ +typedef void (*virEventTimeoutFreeFunc)(void *opaque); + +/** * virEventAddTimeoutFunc: * @timeout: The timeout to monitor * @cb: the callback to call when timeout has expired + * @ff: the callback invoked to free opaque data blob * @opaque: user data to pass to the callback * * Part of the EventImpl, this user-defined callback handles adding an * event timeout. * + * If the opaque user data requires free'ing when the handle + * is unregistered, then a 2nd callback can be supplied for + * this purpose. + * * Returns a timer value */ -typedef int (*virEventAddTimeoutFunc)(int timeout, virEventTimeoutCallback cb, +typedef int (*virEventAddTimeoutFunc)(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, void *opaque); /** @@ -1207,6 +1247,10 @@ typedef void (*virEventUpdateTimeoutFunc * * Part of the EventImpl, this user-defined callback removes a timer * + * If a virEventTimeoutFreeFunc was supplied when the handle was + * registered, it will be invoked some time during, or after this + * function call, when it is safe to release the user data. + * * Returns 0 on success, -1 on failure */ typedef int (*virEventRemoveTimeoutFunc)(int timer); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1132,21 +1132,39 @@ typedef void (*virEventHandleCallback)(i typedef void (*virEventHandleCallback)(int watch, int fd, int events, void *opaque); /** + * virEventHandleFreeFunc: + * + * @opaque: user data to be free'd + * + * Callback invoked when memory associated with a file handle + * watch is being freed. Will be passed a pointer to the application's + * opaque data blob. + */ +typedef void (*virEventHandleFreeFunc)(void *opaque); + +/** * virEventAddHandleFunc: * @fd: file descriptor to listen on * @event: bitset of events on which to fire the callback - * @cb: the callback to be called + * @cb: the callback to be called when an event occurrs + * @ff: the callback invoked to free opaque data blob * @opaque: user data to pass to the callback * * Part of the EventImpl, this callback Adds a file handle callback to * listen for specific events. The same file handle can be registered * multiple times provided the requested event sets are non-overlapping * + * If the opaque user data requires free'ing when the handle + * is unregistered, then a 2nd callback can be supplied for + * this purpose. + * * Returns a handle watch number to be used for updating * and unregistering for events */ typedef int (*virEventAddHandleFunc)(int fd, int event, - virEventHandleCallback cb, void *opaque); + virEventHandleCallback cb, + virEventHandleFreeFunc ff, + void *opaque); /** * virEventUpdateHandleFunc: @@ -1163,7 +1181,11 @@ typedef void (*virEventUpdateHandleFunc) * @watch: file descriptor watch to stop listening on * * Part of the EventImpl, this user-provided callback is notified when - * an fd is no longer being listened on + * an fd is no longer being listened on. + * + * If a virEventHandleFreeFunc was supplied when the handle was + * registered, it will be invoked some time during, or after this + * function call, when it is safe to release the user data. */ typedef int (*virEventRemoveHandleFunc)(int watch); @@ -1178,17 +1200,35 @@ typedef void (*virEventTimeoutCallback)( typedef void (*virEventTimeoutCallback)(int timer, void *opaque); /** + * virEventTimeoutFreeFunc: + * + * @opaque: user data to be free'd + * + * Callback invoked when memory associated with a timer + * is being freed. Will be passed a pointer to the application's + * opaque data blob. + */ +typedef void (*virEventTimeoutFreeFunc)(void *opaque); + +/** * virEventAddTimeoutFunc: * @timeout: The timeout to monitor * @cb: the callback to call when timeout has expired + * @ff: the callback invoked to free opaque data blob * @opaque: user data to pass to the callback * * Part of the EventImpl, this user-defined callback handles adding an * event timeout. * + * If the opaque user data requires free'ing when the handle + * is unregistered, then a 2nd callback can be supplied for + * this purpose. + * * Returns a timer value */ -typedef int (*virEventAddTimeoutFunc)(int timeout, virEventTimeoutCallback cb, +typedef int (*virEventAddTimeoutFunc)(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, void *opaque); /** @@ -1207,6 +1247,10 @@ typedef void (*virEventUpdateTimeoutFunc * * Part of the EventImpl, this user-defined callback removes a timer * + * If a virEventTimeoutFreeFunc was supplied when the handle was + * registered, it will be invoked some time during, or after this + * function call, when it is safe to release the user data. + * * Returns 0 on success, -1 on failure */ typedef int (*virEventRemoveTimeoutFunc)(int timer); diff --git a/python/libvir.c b/python/libvir.c --- a/python/libvir.c +++ b/python/libvir.c @@ -1704,12 +1704,16 @@ static PyObject *removeTimeoutObj = NULL static int -libvirt_virEventAddHandleFunc (int fd ATTRIBUTE_UNUSED, int event ATTRIBUTE_UNUSED, - virEventHandleCallback cb, void *opaque) +libvirt_virEventAddHandleFunc (int fd, + int event, + virEventHandleCallback cb, + virEventHandleFreeFunc ff, + void *opaque) { PyObject *result = NULL; PyObject *python_cb; PyObject *cb_obj; + PyObject *ff_obj; PyObject *opaque_obj; PyObject *cb_args; PyObject *pyobj_args; @@ -1730,11 +1734,13 @@ libvirt_virEventAddHandleFunc (int fd A /* create tuple for cb */ cb_obj = libvirt_virEventHandleCallbackWrap(cb); + ff_obj = libvirt_virEventHandleFreeFuncWrap(ff); opaque_obj = libvirt_virVoidPtrWrap(opaque); - cb_args = PyTuple_New(2); + cb_args = PyTuple_New(3); PyTuple_SetItem(cb_args, 0, cb_obj); - PyTuple_SetItem(cb_args, 1, opaque_obj); + PyTuple_SetItem(cb_args, 1, ff_obj); + PyTuple_SetItem(cb_args, 2, opaque_obj); pyobj_args = PyTuple_New(4); PyTuple_SetItem(pyobj_args, 0, libvirt_intWrap(fd)); @@ -1799,7 +1805,9 @@ libvirt_virEventRemoveHandleFunc(int fd) } static int -libvirt_virEventAddTimeoutFunc(int timeout, virEventTimeoutCallback cb, +libvirt_virEventAddTimeoutFunc(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, void *opaque) { PyObject *result = NULL; @@ -1807,6 +1815,7 @@ libvirt_virEventAddTimeoutFunc(int timeo PyObject *python_cb; PyObject *cb_obj; + PyObject *ff_obj; PyObject *opaque_obj; PyObject *cb_args; PyObject *pyobj_args; @@ -1827,11 +1836,13 @@ libvirt_virEventAddTimeoutFunc(int timeo /* create tuple for cb */ cb_obj = libvirt_virEventTimeoutCallbackWrap(cb); + ff_obj = libvirt_virEventTimeoutFreeFuncWrap(ff); opaque_obj = libvirt_virVoidPtrWrap(opaque); - cb_args = PyTuple_New(2); + cb_args = PyTuple_New(3); PyTuple_SetItem(cb_args, 0, cb_obj); - PyTuple_SetItem(cb_args, 1, opaque_obj); + PyTuple_SetItem(cb_args, 1, ff_obj); + PyTuple_SetItem(cb_args, 2, opaque_obj); pyobj_args = PyTuple_New(3); diff --git a/python/libvirt_wrap.h b/python/libvirt_wrap.h --- a/python/libvirt_wrap.h +++ b/python/libvirt_wrap.h @@ -103,6 +103,8 @@ PyObject * libvirt_virStorageVolPtrWrap( PyObject * libvirt_virStorageVolPtrWrap(virStorageVolPtr node); PyObject * libvirt_virEventHandleCallbackWrap(virEventHandleCallback node); PyObject * libvirt_virEventTimeoutCallbackWrap(virEventTimeoutCallback node); +PyObject * libvirt_virEventHandleFreeFuncWrap(virEventHandleFreeFunc node); +PyObject * libvirt_virEventTimeoutFreeFuncWrap(virEventTimeoutFreeFunc node); PyObject * libvirt_virVoidPtrWrap(void* node); /* Provide simple macro statement wrappers (adapted from GLib, in turn from Perl): diff --git a/python/types.c b/python/types.c --- a/python/types.c +++ b/python/types.c @@ -196,6 +196,38 @@ libvirt_virEventTimeoutCallbackWrap(virE } PyObject * +libvirt_virEventHandleFreeFuncWrap(virEventHandleFreeFunc node) +{ + PyObject *ret; + + if (node == NULL) { + Py_INCREF(Py_None); + printf("%s: WARNING - Wrapping None\n", __FUNCTION__); + return (Py_None); + } + ret = + PyCObject_FromVoidPtrAndDesc((void *) node, (char *) "virEventHandleFreeFunc", + NULL); + return (ret); +} + +PyObject * +libvirt_virEventTimeoutFreeFuncWrap(virEventTimeoutFreeFunc node) +{ + PyObject *ret; + + if (node == NULL) { + printf("%s: WARNING - Wrapping None\n", __FUNCTION__); + Py_INCREF(Py_None); + return (Py_None); + } + ret = + PyCObject_FromVoidPtrAndDesc((void *) node, (char *) "virEventTimeoutFreeFunc", + NULL); + return (ret); +} + +PyObject * libvirt_virVoidPtrWrap(void* node) { PyObject *ret; diff --git a/qemud/event.c b/qemud/event.c --- a/qemud/event.c +++ b/qemud/event.c @@ -41,6 +41,7 @@ struct virEventHandle { int fd; int events; virEventHandleCallback cb; + virEventHandleFreeFunc ff; void *opaque; int deleted; }; @@ -51,6 +52,7 @@ struct virEventTimeout { int frequency; unsigned long long expiresAt; virEventTimeoutCallback cb; + virEventTimeoutFreeFunc ff; void *opaque; int deleted; }; @@ -83,7 +85,9 @@ static int nextTimer = 0; * NB, it *must* be safe to call this from within a callback * For this reason we only ever append to existing list. */ -int virEventAddHandleImpl(int fd, int events, virEventHandleCallback cb, +int virEventAddHandleImpl(int fd, int events, + virEventHandleCallback cb, + virEventHandleFreeFunc ff, void *opaque) { EVENT_DEBUG("Add handle %d %d %p %p", fd, events, cb, opaque); if (eventLoop.handlesCount == eventLoop.handlesAlloc) { @@ -100,6 +104,7 @@ int virEventAddHandleImpl(int fd, int ev eventLoop.handles[eventLoop.handlesCount].events = virEventHandleTypeToPollEvent(events); eventLoop.handles[eventLoop.handlesCount].cb = cb; + eventLoop.handles[eventLoop.handlesCount].ff = ff; eventLoop.handles[eventLoop.handlesCount].opaque = opaque; eventLoop.handles[eventLoop.handlesCount].deleted = 0; @@ -147,7 +152,10 @@ int virEventRemoveHandleImpl(int watch) * NB, it *must* be safe to call this from within a callback * For this reason we only ever append to existing list. */ -int virEventAddTimeoutImpl(int frequency, virEventTimeoutCallback cb, void *opaque) { +int virEventAddTimeoutImpl(int frequency, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, + void *opaque) { struct timeval now; EVENT_DEBUG("Adding timer %d with %d ms freq", nextTimer, frequency); if (gettimeofday(&now, NULL) < 0) { @@ -166,6 +174,7 @@ int virEventAddTimeoutImpl(int frequency eventLoop.timeouts[eventLoop.timeoutsCount].timer = nextTimer++; eventLoop.timeouts[eventLoop.timeoutsCount].frequency = frequency; eventLoop.timeouts[eventLoop.timeoutsCount].cb = cb; + eventLoop.timeouts[eventLoop.timeoutsCount].ff = ff; eventLoop.timeouts[eventLoop.timeoutsCount].opaque = opaque; eventLoop.timeouts[eventLoop.timeoutsCount].deleted = 0; eventLoop.timeouts[eventLoop.timeoutsCount].expiresAt = @@ -393,6 +402,9 @@ static int virEventCleanupTimeouts(void) } EVENT_DEBUG("Purging timeout %d with id %d", i, eventLoop.timeouts[i].timer); + if (eventLoop.timeouts[i].ff) + (eventLoop.timeouts[i].ff)(eventLoop.timeouts[i].opaque); + if ((i+1) < eventLoop.timeoutsCount) { memmove(eventLoop.timeouts+i, eventLoop.timeouts+i+1, @@ -428,6 +440,9 @@ static int virEventCleanupHandles(void) i++; continue; } + + if (eventLoop.handles[i].ff) + (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque); if ((i+1) < eventLoop.handlesCount) { memmove(eventLoop.handles+i, diff --git a/qemud/event.h b/qemud/event.h --- a/qemud/event.h +++ b/qemud/event.h @@ -36,7 +36,9 @@ * * returns -1 if the file handle cannot be registered, 0 upon success */ -int virEventAddHandleImpl(int fd, int events, virEventHandleCallback cb, +int virEventAddHandleImpl(int fd, int events, + virEventHandleCallback cb, + virEventHandleFreeFunc ff, void *opaque); /** @@ -71,7 +73,10 @@ int virEventRemoveHandleImpl(int watch); * returns -1 if the file handle cannot be registered, a positive * integer timer id upon success */ -int virEventAddTimeoutImpl(int frequency, virEventTimeoutCallback cb, void *opaque); +int virEventAddTimeoutImpl(int frequency, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, + void *opaque); /** * virEventUpdateTimeoutImpl: change frequency for a timer diff --git a/qemud/mdns.c b/qemud/mdns.c --- a/qemud/mdns.c +++ b/qemud/mdns.c @@ -238,6 +238,12 @@ static void libvirtd_mdns_watch_dispatch w->callback(w, fd, fd_events, w->userdata); } +static void libvirtd_mdns_watch_dofree(void *w) +{ + VIR_FREE(w); +} + + static AvahiWatch *libvirtd_mdns_watch_new(const AvahiPoll *api ATTRIBUTE_UNUSED, int fd, AvahiWatchEvent event, AvahiWatchCallback cb, void *userdata) { @@ -254,7 +260,9 @@ static AvahiWatch *libvirtd_mdns_watch_n AVAHI_DEBUG("New handle %p FD %d Event %d", w, w->fd, event); hEvents = virPollEventToEventHandleType(event); if ((w->watch = virEventAddHandleImpl(fd, hEvents, - libvirtd_mdns_watch_dispatch, w)) < 0) { + libvirtd_mdns_watch_dispatch, + libvirtd_mdns_watch_dofree, + w)) < 0) { VIR_FREE(w); return NULL; } @@ -278,7 +286,6 @@ static void libvirtd_mdns_watch_free(Ava { AVAHI_DEBUG("Free handle %p %d", w, w->fd); virEventRemoveHandleImpl(w->watch); - VIR_FREE(w); } static void libvirtd_mdns_timeout_dispatch(int timer ATTRIBUTE_UNUSED, void *opaque) @@ -287,6 +294,11 @@ static void libvirtd_mdns_timeout_dispat AVAHI_DEBUG("Dispatch timeout %p %d", t, timer); virEventUpdateTimeoutImpl(t->timer, -1); t->callback(t, t->userdata); +} + +static void libvirtd_mdns_timeout_dofree(void *t) +{ + VIR_FREE(t); } static AvahiTimeout *libvirtd_mdns_timeout_new(const AvahiPoll *api ATTRIBUTE_UNUSED, @@ -319,7 +331,10 @@ static AvahiTimeout *libvirtd_mdns_timeo timeout = -1; } - t->timer = virEventAddTimeoutImpl(timeout, libvirtd_mdns_timeout_dispatch, t); + t->timer = virEventAddTimeoutImpl(timeout, + libvirtd_mdns_timeout_dispatch, + libvirtd_mdns_timeout_dofree, + t); t->callback = cb; t->userdata = userdata; @@ -358,7 +373,6 @@ static void libvirtd_mdns_timeout_free(A { AVAHI_DEBUG("Free timeout %p", t); virEventRemoveTimeoutImpl(t->timer); - VIR_FREE(t); } diff --git a/qemud/qemud.c b/qemud/qemud.c --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -540,6 +540,7 @@ static int qemudListenUnix(struct qemud_ VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, qemudDispatchServerEvent, + NULL, server)) < 0) { qemudLog(QEMUD_ERR, "%s", _("Failed to add server event callback")); @@ -672,6 +673,7 @@ remoteListenTCP (struct qemud_server *se VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, qemudDispatchServerEvent, + NULL, server)) < 0) { qemudLog(QEMUD_ERR, "%s", _("Failed to add server event callback")); goto cleanup; @@ -1655,6 +1657,7 @@ static int qemudRegisterClientEvent(stru mode | VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, qemudDispatchClientEvent, + NULL, server)) < 0) return -1; @@ -1721,7 +1724,10 @@ static int qemudRunLoop(struct qemud_ser * shutdown after timeout seconds */ if (timeout > 0 && !virStateActive() && !server->clients) { - timerid = virEventAddTimeoutImpl(timeout*1000, qemudInactiveTimer, server); + timerid = virEventAddTimeoutImpl(timeout*1000, + qemudInactiveTimer, + NULL, + server); qemudDebug("Scheduling shutdown timer %d", timerid); } @@ -2270,6 +2276,7 @@ int main(int argc, char **argv) { if (virEventAddHandleImpl(sigpipe[0], VIR_EVENT_HANDLE_READABLE, qemudDispatchSignalEvent, + NULL, server) < 0) { qemudLog(QEMUD_ERR, "%s", _("Failed to register callback for signal pipe")); diff --git a/src/event.c b/src/event.c --- a/src/event.c +++ b/src/event.c @@ -34,12 +34,15 @@ static virEventUpdateTimeoutFunc updateT static virEventUpdateTimeoutFunc updateTimeoutImpl = NULL; static virEventRemoveTimeoutFunc removeTimeoutImpl = NULL; -int virEventAddHandle(int fd, int events, virEventHandleCallback cb, +int virEventAddHandle(int fd, + int events, + virEventHandleCallback cb, + virEventHandleFreeFunc ff, void *opaque) { if (!addHandleImpl) return -1; - return addHandleImpl(fd, events, cb, opaque); + return addHandleImpl(fd, events, cb, ff, opaque); } void virEventUpdateHandle(int watch, int events) { @@ -53,11 +56,14 @@ int virEventRemoveHandle(int watch) { return removeHandleImpl(watch); } -int virEventAddTimeout(int timeout, virEventTimeoutCallback cb, void *opaque) { +int virEventAddTimeout(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, + void *opaque) { if (!addTimeoutImpl) return -1; - return addTimeoutImpl(timeout, cb, opaque); + return addTimeoutImpl(timeout, cb, ff, opaque); } void virEventUpdateTimeout(int timer, int timeout) { diff --git a/src/event.h b/src/event.h --- a/src/event.h +++ b/src/event.h @@ -34,7 +34,9 @@ * * returns -1 if the file handle cannot be registered, 0 upon success */ -int virEventAddHandle(int fd, int events, virEventHandleCallback cb, +int virEventAddHandle(int fd, int events, + virEventHandleCallback cb, + virEventHandleFreeFunc ff, void *opaque); /** @@ -69,7 +71,10 @@ int virEventRemoveHandle(int watch); * returns -1 if the file handle cannot be registered, a positive * integer timer id upon success */ -int virEventAddTimeout(int frequency, virEventTimeoutCallback cb, void *opaque); +int virEventAddTimeout(int frequency, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, + void *opaque); /** * virEventUpdateTimeoutImpl: change frequency for a timer diff --git a/src/lxc_driver.c b/src/lxc_driver.c --- a/src/lxc_driver.c +++ b/src/lxc_driver.c @@ -820,6 +820,7 @@ static int lxcVmStart(virConnectPtr conn vm->monitor, VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, lxcMonitorEvent, + NULL, driver)) < 0) { lxcVmTerminate(conn, driver, vm, 0); goto cleanup; diff --git a/src/qemu_driver.c b/src/qemu_driver.c --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -952,12 +952,14 @@ static int qemudStartVMDaemon(virConnect VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, qemudDispatchVMEvent, + NULL, driver)) < 0) || ((vm->stderr_watch = virEventAddHandle(vm->stderr_fd, VIR_EVENT_HANDLE_READABLE | VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, qemudDispatchVMEvent, + NULL, driver)) < 0) || (qemudWaitForMonitor(conn, driver, vm) < 0) || (qemudDetectVcpuPIDs(conn, driver, vm) < 0) || diff --git a/src/remote_internal.c b/src/remote_internal.c --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -762,6 +762,7 @@ doRemoteOpen (virConnectPtr conn, VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP, remoteDomainEventFired, + NULL, conn)) < 0) { DEBUG0("virEventAddHandle failed: No addHandleImpl defined." " continuing without events."); @@ -770,6 +771,7 @@ doRemoteOpen (virConnectPtr conn, DEBUG0("Adding Timeout for remote event queue flushing"); if ( (priv->eventFlushTimer = virEventAddTimeout(-1, remoteDomainEventQueueFlush, + NULL, conn)) < 0) { DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. " "continuing without events."); -- |: 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 :|

On Mon, Nov 17, 2008 at 05:02:40PM +0000, Daniel P. Berrange wrote:
When registering for a file descriptor event, or timer events, the event callback has an associated 'void *opaque' data blob. When removing a registered event, the removal may be done asynchronously to allow safe removal from within a callback. This means that it is not safe for the application to assume they can free the 'void *opaque' data immediately after calling virEventRemoveHandle/Timer. So, we extend the AddHandle/Timer method to allow a 2nd callback to be provided. This callback is used to free the 'void *opaque' data at the appropriate (safe) point in time.
okay, it makes more sense to provide the API framework on how to free the data
diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h --- a/include/libvirt/libvirt.h +++ b/include/libvirt/libvirt.h @@ -1132,21 +1132,39 @@ typedef void (*virEventHandleCallback)(i typedef void (*virEventHandleCallback)(int watch, int fd, int events, void *opaque);
/** + * virEventHandleFreeFunc: + * + * @opaque: user data to be free'd + * + * Callback invoked when memory associated with a file handle + * watch is being freed. Will be passed a pointer to the application's + * opaque data blob. + */ +typedef void (*virEventHandleFreeFunc)(void *opaque); + +/** * virEventAddHandleFunc: * @fd: file descriptor to listen on * @event: bitset of events on which to fire the callback - * @cb: the callback to be called + * @cb: the callback to be called when an event occurrs + * @ff: the callback invoked to free opaque data blob * @opaque: user data to pass to the callback * * Part of the EventImpl, this callback Adds a file handle callback to * listen for specific events. The same file handle can be registered * multiple times provided the requested event sets are non-overlapping * + * If the opaque user data requires free'ing when the handle + * is unregistered, then a 2nd callback can be supplied for + * this purpose. + * * Returns a handle watch number to be used for updating * and unregistering for events */ typedef int (*virEventAddHandleFunc)(int fd, int event, - virEventHandleCallback cb, void *opaque); + virEventHandleCallback cb, + virEventHandleFreeFunc ff, + void *opaque);
/** * virEventUpdateHandleFunc: @@ -1163,7 +1181,11 @@ typedef void (*virEventUpdateHandleFunc) * @watch: file descriptor watch to stop listening on * * Part of the EventImpl, this user-provided callback is notified when - * an fd is no longer being listened on + * an fd is no longer being listened on. + * + * If a virEventHandleFreeFunc was supplied when the handle was + * registered, it will be invoked some time during, or after this + * function call, when it is safe to release the user data. */ typedef int (*virEventRemoveHandleFunc)(int watch);
@@ -1178,17 +1200,35 @@ typedef void (*virEventTimeoutCallback)( typedef void (*virEventTimeoutCallback)(int timer, void *opaque);
/** + * virEventTimeoutFreeFunc: + * + * @opaque: user data to be free'd + * + * Callback invoked when memory associated with a timer + * is being freed. Will be passed a pointer to the application's + * opaque data blob. + */ +typedef void (*virEventTimeoutFreeFunc)(void *opaque); + +/** * virEventAddTimeoutFunc: * @timeout: The timeout to monitor * @cb: the callback to call when timeout has expired + * @ff: the callback invoked to free opaque data blob * @opaque: user data to pass to the callback * * Part of the EventImpl, this user-defined callback handles adding an * event timeout. * + * If the opaque user data requires free'ing when the handle + * is unregistered, then a 2nd callback can be supplied for + * this purpose. + * * Returns a timer value */ -typedef int (*virEventAddTimeoutFunc)(int timeout, virEventTimeoutCallback cb, +typedef int (*virEventAddTimeoutFunc)(int timeout, + virEventTimeoutCallback cb, + virEventTimeoutFreeFunc ff, void *opaque);
/** @@ -1207,6 +1247,10 @@ typedef void (*virEventUpdateTimeoutFunc * * Part of the EventImpl, this user-defined callback removes a timer * + * If a virEventTimeoutFreeFunc was supplied when the handle was + * registered, it will be invoked some time during, or after this + * function call, when it is safe to release the user data. + * * Returns 0 on success, -1 on failure */ typedef int (*virEventRemoveTimeoutFunc)(int timer);
Okay, the API change looks fine, no problem for me. and everything else seems to be a direct set of change related to the extra callback function, +1 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On Wed, Nov 19, 2008 at 02:21:40PM +0100, Daniel Veillard wrote:
On Mon, Nov 17, 2008 at 05:02:40PM +0000, Daniel P. Berrange wrote:
When registering for a file descriptor event, or timer events, the event callback has an associated 'void *opaque' data blob. When removing a registered event, the removal may be done asynchronously to allow safe removal from within a callback. This means that it is not safe for the application to assume they can free the 'void *opaque' data immediately after calling virEventRemoveHandle/Timer. So, we extend the AddHandle/Timer method to allow a 2nd callback to be provided. This callback is used to free the 'void *opaque' data at the appropriate (safe) point in time.
okay, it makes more sense to provide the API framework on how to free the data
I've committed this change, but using the generic virFreeCallback typedef, instead of my custom one. Also I put the virFreeCallback as the last arg, to match the style of David Lively's equivalent patch to the virDomainEventRegister call. 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 :|
participants (3)
-
Daniel Berrange
-
Daniel P. Berrange
-
Daniel Veillard