[libvirt] [PATCH 00/12] Domain Events - Round 2
The following patch series implements the Events API discussed here previously in this thread: http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html By Daniel B's advice - I have broken this patch into the following: events-01-public-api Changes to libvirt.h, and libvirt.c events-02-internal-api Changes to internal.h and event code events-03-qemud Changes to the daemon events-04-qemud-rpc-protocol Changes to the qemud rpc protocol events-05-driver.patch Changes to the driver API events-06-driver-qemu Changes to the qemu driver events-07-driver-remote Changes to the remote driver events-08-driver-lxc Minor changes to LXC driver structure to prevent compile errors events-09-driver-openvz Minor changes to openvz driver structure to prevent compile errors events-10-driver-test Minor changes to test driver structure to prevent compile errors events-11-example-testapp Test app, and infrastructure changes for an example dir events-12-python-ignore.patch Add functions to be ignored, for now For now, domain events are only emitted from qemu/kvm guests.
This patch does the following: -implements the Event register/deregister code -Adds some callback lists, and queue functions used by drivers -Move EventImpl definitions into the public include/libvirt/libvirt.h | 59 +++++++++ include/libvirt/libvirt.h.in | 59 +++++++++ src/libvirt.c | 256 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_sym.version | 9 + 4 files changed, 381 insertions(+), 2 deletions(-)
On Fri, Oct 17, 2008 at 11:53:53AM -0400, Ben Guthro wrote:
This patch does the following: -implements the Event register/deregister code -Adds some callback lists, and queue functions used by drivers -Move EventImpl definitions into the public
include/libvirt/libvirt.h | 59 +++++++++ include/libvirt/libvirt.h.in | 59 +++++++++ src/libvirt.c | 256 +++++++++++++++++++++++++++++++++++++++++++ src/libvirt_sym.version | 9 + 4 files changed, 381 insertions(+), 2 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 3624367..6af5329 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in + +/** + * virEventHandleCallback: callback for receiving file handle events + * + * @fd: file handle on which the event occurred + * @events: bitset of events from POLLnnn constants + * @opaque: user data registered with handle + */ +typedef void (*virEventHandleCallback)(int fd, int events, void *opaque); + +typedef int (*virEventAddHandleFunc)(int, int, virEventHandleCallback, void *); +typedef void (*virEventUpdateHandleFunc)(int, int); +typedef int (*virEventRemoveHandleFunc)(int);
For the handle here, we need to define some constants for the events. The current internal impl assumed that the events parameter was just using the POLLnn constants, but not all users of libvirt will neccessarily use poll() for their event loop. So I think we should define something like enum { VIR_EVENT_HANDLE_READABLE = (1 << 0) VIR_EVENT_HANDLE_WRITABLE = (1 << 1) VIR_EVENT_HANDLE_ERROR = (1 << 2) VIR_EVENT_HANDLE_HANGUP = (1 << 2) };
+ +/** + * virDispatchDomainEvent: + * @dom: the domain + * @event: the domain event code + * + * Internal function by which drivers to dispatch domain events. + */ +void virDispatchDomainEvent(virDomainPtr dom, + int event) +{ + if (dom->conn && dom->conn->driver && + dom->conn->driver->domainEventDispatch) + dom->conn->driver->domainEventDispatch(dom, event); +}
I don't think this helper method is needed in libvirt.c - i'll show why in my reply to the QEMU driver patch. Other than these two points, the rest of this patch looks sound. 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 :|
On Fri, Oct 17, 2008 at 11:53:53AM -0400, Ben Guthro wrote:
This patch does the following: -implements the Event register/deregister code -Adds some callback lists, and queue functions used by drivers -Move EventImpl definitions into the public
Basically looks fine with however a few things in-line
+ +typedef int (*virConnectDomainEventCallback)(virConnectPtr conn, + virDomainPtr dom, + int event, + void *opaque); + +int virConnectDomainEventRegister(virConnectPtr conn, + virConnectDomainEventCallback cb, + void *opaque); + +int virConnectDomainEventDeregister(virConnectPtr conn, + virConnectDomainEventCallback cb);
Please put comments for all function type definitions this is needed for generating the docs, bindings, etc... The comment for the functions themselves should be set in the C file. Basically the comment for a construct should be at the place where the construct is defined.
+/** + * virEventHandleCallback: callback for receiving file handle events + * + * @fd: file handle on which the event occurred + * @events: bitset of events from POLLnnn constants + * @opaque: user data registered with handle + */ +typedef void (*virEventHandleCallback)(int fd, int events, void *opaque); + +typedef int (*virEventAddHandleFunc)(int, int, virEventHandleCallback, void *); +typedef void (*virEventUpdateHandleFunc)(int, int); +typedef int (*virEventRemoveHandleFunc)(int);
Also give name to all parameters, even if C allows passing only the type, we really need names and comments for all of them
+ +typedef int (*virEventAddTimeoutFunc)(int, virEventTimeoutCallback, void *); +typedef void (*virEventUpdateTimeoutFunc)(int, int); +typedef int (*virEventRemoveTimeoutFunc)(int);
Same thing And similar for libvirt.h.in
diff --git a/src/libvirt.c b/src/libvirt.c [...] +/** + * virConnectDomainEventDeregister: + * @conn: pointer to the connection + * @cb: callback to the function handling domain events + * + * Removes a Domain Event Callback + * + * Returns 0 on success, -1 on failure + */ +int virConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback cb) +{
the following comment is part of the function description, move it in the function comment block, and please keep lines less than 80 columns. Also stylistic we use int virFunction(...) { for indenting and this kind of things the HACKING file has an indent description which can be used to automate most of this :-)
+ /* De-registering for a domain callback will disable delivery of this event type*/
For any part of the public API, all parameters must be checked as thorougthly as possible, there is macros to test active connections reuse them. Maybe cb should be checked for NULL, if it is allowed then the function description should indicate the semantic. of passing NULL.
+ if (conn->driver && conn->driver->domainEventDeregister) + return conn->driver->domainEventDeregister (conn, cb); + + return -1; +} + +/** + * virDispatchDomainEvent: + * @dom: the domain + * @event: the domain event code + * + * Internal function by which drivers to dispatch domain events. + */ +void virDispatchDomainEvent(virDomainPtr dom, + int event) +{
parameter check
+ if (dom->conn && dom->conn->driver && + dom->conn->driver->domainEventDispatch) + dom->conn->driver->domainEventDispatch(dom, event); +} + +/** + * virDomainEventCallbackListRemove:
missing @conn ... not important as it's private but for completion
+ * @cbList: the list + * @callback: the callback to remove + * [...] +/** + * virDomainEventCallbackListAdd:
same thing
+ * @cbList: the list + * @callback: the callback to add + * @opaque: opaque data tio pass to callback + * + * Internal function to add a callback from a virDomainEventCallbackListPtr + */ +int __virDomainEventCallbackListAdd(virConnectPtr conn, + virDomainEventCallbackListPtr cbList, + virConnectDomainEventCallback callback, + void *opaque) +{ + virDomainEventCallbackPtr event; + int n; + + /* Check incoming */ + if ( !cbList ) { + return -1; + } + + /* check if we already have this callback on our list */ + for( n=0; n < cbList->count; n++ ) + { + if(cbList->callbacks[n]->cb == callback && + conn == cbList->callbacks[n]->conn) { + DEBUG0("WARNING: Callback already tracked"); + return -1; + } + }
again, purely stylistic but libvirt code uses for (n=0; n < cbList->count; n++) { if ((cbList->callbacks[n]->cb == callback) && (conn == cbList->callbacks[n]->conn)) { i.e. different spacing, fully parenthesize testing expressings instead of relying on operator priorities
+ /* Allocate new event */ + if (VIR_ALLOC(event) < 0) { + DEBUG0("Error allocating event"); + return -1; + } + event->conn = conn; + event->cb = callback; + event->opaque = opaque; + + /* Make space on list */ + n = cbList->count; + if (VIR_REALLOC_N(cbList->callbacks, + n + 1) < 0) {
test fits in one line :-)
+ DEBUG0("Error reallocating list"); + VIR_FREE(event); + return -1; + } + + event->conn->refs++; + + cbList->callbacks[n] = event; + cbList->count++; + return 0; +} + +/** + * virDomainEventQueueFree: + * @queue: fron of queue + * + * Free the memory in the queue. We process this like a list here + */ +void virDomainEventQueueFree(virDomainEventQueuePtr queue) +{ + int i;
need to check all parameters. For structure passed down to the API as pointers, please add a magic field, a definition for the magic value and a macro VIR_IS_DOMAIN_EVENT_QUEUE allowing to trivially test the input parameter.
+/** + * virDomainEventCallbackQueuePop: + * @evtQueue: the queue of events + * + * Internal function to pop off, and return the front of the queue
if it's really internal it should be made static, or start with __
+ * NOTE: The caller is responsible for freeing the returned object + * + * Returns: virDomainEventPtr on success NULL on failure. + */ +virDomainEventPtr virDomainEventCallbackQueuePop(virDomainEventQueuePtr evtQueue) +{ + virDomainEventPtr ret; + + if(!evtQueue || evtQueue->count == 0 ) + return NULL; + + ret = evtQueue->events[0]; + + memmove(evtQueue->events, + evtQueue->events + 1, + sizeof(*(evtQueue->events)) * + (evtQueue->count - 1)); + + if (VIR_REALLOC_N(evtQueue->events, + evtQueue->count - 1) < 0) { + ; /* Failure to reduce memory allocation isn't fatal */ + }
While this is correct code, I start to wonder if the event queue shoudn't avoid realloc'ing twice per event, that's probably one of the case where a size of the event queue allocated should be kept.
+ evtQueue->count--; + + return ret; +} + +/** + * virDomainEventCallbackQueuePush: + * @evtQueue: the dom event queue + * @dom: the domain to add + * @event: the event to add + * + * Internal function to push onto the back of an virDomainEventQueue + * + * Returns: 0 on success, -1 on failure + */ +int virDomainEventCallbackQueuePush(virDomainEventQueuePtr evtQueue, + virDomainPtr dom, + virDomainEventType event, + virConnectDomainEventCallback cb, + void *opaque) +{ + virDomainEventPtr domEvent; + + /* Check incoming */ + if ( !evtQueue ) { + return -1;
Ah :-) , good idea, but implement the magic + macro thing for testing :-)
+ } + + /* Allocate new event */ + if (VIR_ALLOC(domEvent) < 0) { + DEBUG0("Error allocating event"); + return -1; + } + domEvent->dom = dom; + domEvent->event = event; + domEvent->cb = cb; + domEvent->opaque = opaque; + + /* Make space on queue */ + if (VIR_REALLOC_N(evtQueue->events, + evtQueue->count + 1) < 0) { + DEBUG0("Error reallocating queue"); + VIR_FREE(domEvent); + return -1; + }
Do we really want to allow the event queue to grow indefinitely if the events are not read. I guess something is needed here, or at least a comment with a TODO
+ evtQueue->events[evtQueue->count] = domEvent; + evtQueue->count++; + return 0; +} diff --git a/src/libvirt_sym.version b/src/libvirt_sym.version index 3cc4505..5776cf9 100644 --- a/src/libvirt_sym.version +++ b/src/libvirt_sym.version @@ -147,6 +147,10 @@ virStorageVolGetXMLDesc; virStorageVolGetPath;
+ virEventRegisterImpl; + virConnectDomainEventRegister; + virConnectDomainEventDeregister; + /* Symbols with __ are private only for use by the libvirtd daemon. They are not part of stable ABI @@ -167,8 +171,6 @@ __virGetStoragePool; __virGetStorageVol;
- __virEventRegisterImpl; - __virStateInitialize; __virStateCleanup; __virStateReload; @@ -198,5 +200,8 @@ __virReallocN; __virFree;
+ __virDomainEventCallbackListFree; + __virDomainEventCallbackListAdd; + __virDomainEventCallbackListRemove; local: *; };
Overall the principle of the APIs sounds fine but there is some cleaning needed before really exposing them. 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/
This code changes the daemaon to: use the pulic def of virEventRegisterImpl Add functionality to dispatch events to connected remote drivers qemud.c | 28 ++++++++--- qemud.h | 14 +++++ remote.c | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 7 deletions(-)
This patch -Removes EventImpl from being a private function -Introduces the virDomainEventCallbackListPtr, and virDomainEventQueuePtr objects proxy/Makefile.am | 1 src/event.c | 12 +++++----- src/event.h | 37 ------------------------------- src/internal.h | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 42 deletions(-)
On Fri, Oct 17, 2008 at 11:56:54AM -0400, Ben Guthro wrote:
This patch -Removes EventImpl from being a private function -Introduces the virDomainEventCallbackListPtr, and virDomainEventQueuePtr objects
proxy/Makefile.am | 1 src/event.c | 12 +++++----- src/event.h | 37 ------------------------------- src/internal.h | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 42 deletions(-)
This patch looks OK to me. 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 :|
Changes to the RPC protocol remote_dispatch_localvars.h | 3 +++ remote_dispatch_proc_switch.h | 18 ++++++++++++++++++ remote_dispatch_prototypes.h | 3 +++ remote_protocol.c | 35 +++++++++++++++++++++++++++++++++++ remote_protocol.h | 28 ++++++++++++++++++++++++++++ remote_protocol.x | 23 ++++++++++++++++++++++- 6 files changed, 109 insertions(+), 1 deletion(-)
On Fri, Oct 17, 2008 at 11:58:15AM -0400, Ben Guthro wrote:
Changes to the RPC protocol
remote_dispatch_localvars.h | 3 +++ remote_dispatch_proc_switch.h | 18 ++++++++++++++++++ remote_dispatch_prototypes.h | 3 +++ remote_protocol.c | 35 +++++++++++++++++++++++++++++++++++ remote_protocol.h | 28 ++++++++++++++++++++++++++++ remote_protocol.x | 23 ++++++++++++++++++++++- 6 files changed, 109 insertions(+), 1 deletion(-)
[snip generated code]
diff --git a/qemud/remote_protocol.x b/qemud/remote_protocol.x index f1bd9ff..5981702 100644 --- a/qemud/remote_protocol.x +++ b/qemud/remote_protocol.x @@ -965,6 +965,23 @@ struct remote_storage_vol_get_path_ret { remote_nonnull_string name; };
+/* Events */ +struct remote_domain_events_register_args { + unsigned long int callback; /* To store a client pointer */ + unsigned long int user_data; /* For the remote callback opaque data */ +}; + +struct remote_domain_events_deregister_args { + unsigned long int callback; /* To store a client pointer */ +}; + +struct remote_domain_event_ret { + remote_nonnull_domain dom; + int event; + unsigned long int callback; + unsigned long int user_data; +};
Using a 'unsigned long int' field to transmit the raw pointer feels a little wrong to me. Could we have the client side pass a simple integer 'token' when registering / unregistering, and have that 'token' passed back by the server in the actual event. The client could use this token to lookup the callback and user_data. 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 :|
On Sun, 2008-10-19 at 20:22 +0100, Daniel P. Berrange wrote:
On Fri, Oct 17, 2008 at 11:58:15AM -0400, Ben Guthro wrote:
Changes to the RPC protocol
+struct remote_domain_event_ret { + remote_nonnull_domain dom; + int event; + unsigned long int callback; + unsigned long int user_data; +};
Using a 'unsigned long int' field to transmit the raw pointer feels a little wrong to me. Could we have the client side pass a simple integer 'token' when registering / unregistering, and have that 'token' passed back by the server in the actual event. The client could use this token to lookup the callback and user_data.
Hold on. We can (and IMO should) quite easily avoid both this lookup and the passing of the callback pointer to the server: Suppose we have the same client registered for two different domain event callbacks. In the current patch, the server will send two RPCs per event, one for each callback (which the client then unmarshals, casts, and calls). But what if we sent just one RPC per event (& per client) and had the client walk its list of callbacks (which we'll need to track on the client side anyway, if we're not sending the data over the wire)? We *always* make *all* the callbacks on the list, so there's no point in making individual RPCs to fire off each callback individually. This gets rid of the need to send callback/user_data over the wire, and also doesn't require tokenization (which is all just extra overhead in this case). remote_domain_events_register/deregister_args structs will go away. remoteDispatchhDomainEventsRegister/Deregister will simply inc/dec a value, sending events only when the value is >0. While I'm not sure I've described this very well, I feel pretty strongly that it's the right way to go. If my explanation isn't clear, please get back to me ... Dave
This is very similar to what I had in the original patch, where on the server side, we just increment/decrement a callback_registered counter, then keep the list of callbacks/opaque data on the client. The server would then send one rpc to each connected client, whose job it would be to multiplex this out to all registered callbacks. There would still be a "one-for-one" for register/deregister, but this scheme has the following advantages for efficency: a.) Fewer RPC calls (less data on the wire - one-to-many for events firing) b.) Less RPC data passed on register/deregister (no need for cb/user_data) c.) Code is simpler - no need to invent yet another data structure, and list for tracking tokens Daniel - what are your thoughts on this? This is similar to what I had originally implemented, but I'm not sure if you objected to this part of it, or not... Ben David Lively wrote on 10/20/2008 04:05 PM:
On Sun, 2008-10-19 at 20:22 +0100, Daniel P. Berrange wrote:
On Fri, Oct 17, 2008 at 11:58:15AM -0400, Ben Guthro wrote:
Changes to the RPC protocol
+struct remote_domain_event_ret { + remote_nonnull_domain dom; + int event; + unsigned long int callback; + unsigned long int user_data; +}; Using a 'unsigned long int' field to transmit the raw pointer feels a little wrong to me. Could we have the client side pass a simple integer 'token' when registering / unregistering, and have that 'token' passed back by the server in the actual event. The client could use this token to lookup the callback and user_data.
Hold on. We can (and IMO should) quite easily avoid both this lookup and the passing of the callback pointer to the server:
Suppose we have the same client registered for two different domain event callbacks. In the current patch, the server will send two RPCs per event, one for each callback (which the client then unmarshals, casts, and calls).
But what if we sent just one RPC per event (& per client) and had the client walk its list of callbacks (which we'll need to track on the client side anyway, if we're not sending the data over the wire)? We *always* make *all* the callbacks on the list, so there's no point in making individual RPCs to fire off each callback individually. This gets rid of the need to send callback/user_data over the wire, and also doesn't require tokenization (which is all just extra overhead in this case).
remote_domain_events_register/deregister_args structs will go away. remoteDispatchhDomainEventsRegister/Deregister will simply inc/dec a value, sending events only when the value is >0.
While I'm not sure I've described this very well, I feel pretty strongly that it's the right way to go. If my explanation isn't clear, please get back to me ...
Dave
On Tue, Oct 21, 2008 at 08:57:51AM -0400, Ben Guthro wrote:
This is very similar to what I had in the original patch, where on the server side, we just increment/decrement a callback_registered counter, then keep the list of callbacks/opaque data on the client.
The server would then send one rpc to each connected client, whose job it would be to multiplex this out to all registered callbacks.
There would still be a "one-for-one" for register/deregister, but this scheme has the following advantages for efficency: a.) Fewer RPC calls (less data on the wire - one-to-many for events firing) b.) Less RPC data passed on register/deregister (no need for cb/user_data) c.) Code is simpler - no need to invent yet another data structure, and list for tracking tokens
Daniel - what are your thoughts on this? This is similar to what I had originally implemented, but I'm not sure if you objected to this part of it, or not...
The bit I didn't like about the original was the extra logic on the server side of the impl. If we can keep the explicit register & unregister RPC calls, and require the client todo all ref-counting & multiplexing, then I wouldn't mind this approach. eg, on the client side, on the first 'register' API call, send the 'register' RPC call. Then just count subsequent 'register' API calls, and don't bother with the 'register' RPC call. Once the 'unregister' has been called the matching number of times it can send the 'unregister' the RPC call. So the 'remote_internal.c' would have to take care of broadcasting the single incoming event, to all registered listeners of that particular client. 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 :|
Additions to the driver API driver.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
On Fri, Oct 17, 2008 at 12:00:16PM -0400, Ben Guthro wrote:
Additions to the driver API
driver.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/src/driver.h b/src/driver.h index 0540f80..c88dea8 100644 --- a/src/driver.h +++ b/src/driver.h @@ -280,6 +280,22 @@ typedef unsigned long long (*virDrvNodeGetFreeMemory) (virConnectPtr conn);
+typedef int + (*virDrvDomainEventRegister) + (virConnectPtr conn, + void *callback, + void *opaque); + +typedef int + (*virDrvDomainEventDeregister) + (virConnectPtr conn, + void *callback); + +typedef void + (*virDrvDomainEventDispatch) + (virDomainPtr dom, + virDomainEventType evt);
I don't believe this 3rd field is needed here - I'll explain against the QEMU driver patch. 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 :|
Deliver local callbacks in response to remote events remote_internal.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 248 insertions(+), 7 deletions(-)
On Fri, Oct 17, 2008 at 12:02:13PM -0400, Ben Guthro wrote:
Deliver local callbacks in response to remote events
remote_internal.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 248 insertions(+), 7 deletions(-)
@@ -680,6 +689,26 @@ doRemoteOpen (virConnectPtr conn, (xdrproc_t) xdr_void, (char *) NULL) == -1) goto failed;
+ if(VIR_ALLOC(priv->domainEvents)<0) { + error(conn, VIR_ERR_INVALID_ARG, _("Error allocating domainEvents")); + goto failed; + } + + DEBUG0("Adding Handler for remote events"); + /* Set up a callback to listen on the socket data */ + if (virEventAddHandle(priv->sock, + POLLIN | POLLERR | POLLHUP, + remoteDomainEventFired, + conn) < 0) { + DEBUG0("virEventAddHandle failed: No addHandleImpl defined. continuing without events."); + } + + DEBUG0("Adding Timeout for remote event queue flushing"); + if ( (priv->eventFlushTimer = virEventAddTimeout(0, + remoteDomainEventQueueFlush, + conn)) < 0) {
Small bug here - this creates an active timer event, which will fire immediately & forever. Simply change the '0' to a '-1' to register a timeout that's initially disabled, and then use virEventUpdateTimeout to toggle it on/off only when required.
+ +static int remoteDomainEventRegister (virConnectPtr conn, + void *callback, + void *opaque) +{ + struct private_data *priv = conn->privateData; + + /* dispatch an rpc - so the server sde can track + how many callbacks are regstered */ + remote_domain_events_register_args args; + args.callback = (unsigned long)callback; + args.user_data = (unsigned long)opaque;
This relates back to my comment on the remote_protocl.x file - i feel we should probably maintain a generic token, rather than pointing the actual pointers over the wire as ints.
/*----------------------------------------------------------------------*/
@@ -4367,6 +4444,7 @@ call (virConnectPtr conn, struct private_data *priv, really_write (conn, priv, flags & REMOTE_CALL_IN_OPEN, buffer, len-4) == -1) return -1;
+retry_read: /* Read and deserialise length word. */ if (really_read (conn, priv, flags & REMOTE_CALL_IN_OPEN, buffer2, sizeof buffer2) == -1) return -1; @@ -4418,10 +4496,19 @@ call (virConnectPtr conn, struct private_data *priv, return -1; }
- /* If we extend the server to actually send asynchronous messages, then - * we'll need to change this so that it can recognise an asynch - * message being received at this point. - */ + if (hdr.proc == REMOTE_PROC_DOMAIN_EVENT && + hdr.direction == REMOTE_MESSAGE) { + /* An async message has come in while we were waiting for the + * response. Process it to pull it off the wire, and try again + */ + DEBUG0("Encountered an event while waiting for a response"); + + remoteDomainQueueEvent(conn, &xdr);
Need to call virEventUpdateTimeout() to enable the timer here.
+/** + * remoteDomainReadEvent + * + * Read the event data off the wire + */ +static int remoteDomainReadEvent(virConnectPtr conn, XDR *xdr, + virDomainPtr *dom, int *event, + virConnectDomainEventCallback *cb, + void **opaque) +{ + remote_domain_event_ret ret; + memset (&ret, 0, sizeof ret); + + /* unmarshall parameters, and process it*/ + if (! xdr_remote_domain_event_ret(xdr, &ret) ) { + error (conn, VIR_ERR_RPC, _("remoteDomainProcessEvent: unmarshalling ret")); + return -1; + } + + *dom = get_nonnull_domain(conn,ret.dom); + *event = ret.event; + *cb = (virConnectDomainEventCallback)ret.callback; + *opaque = (void *)ret.user_data; + + return 0; +} + +static void remoteDomainProcessEvent(virConnectPtr conn, XDR *xdr) +{ + virDomainPtr dom; + int event; + virConnectDomainEventCallback cb; + void *opaque; + if(!remoteDomainReadEvent(conn, xdr, &dom, &event, &cb, &opaque)) { + DEBUG0("Calling domain event callback (no queue)"); + if(cb) + cb(conn,dom,event,opaque);
Needs to call virDomainFree(dom) to release the object.
+ } +} + +static void remoteDomainQueueEvent(virConnectPtr conn, XDR *xdr) +{ + virDomainPtr dom; + int event; + virConnectDomainEventCallback cb; + void *opaque; + struct private_data *priv = conn->privateData; + + if(!remoteDomainReadEvent(conn, xdr, &dom, &event, &cb, &opaque)) + { + if( virDomainEventCallbackQueuePush(priv->domainEvents, dom, event, cb, opaque) < 0 ) { + DEBUG("%s", "Error adding event to queue"); + } + } +} + +/** remoteDomainEventFired: + * + * The callback for monitoring the remote socket + * for event data + */ +void remoteDomainEventFired(int fd ATTRIBUTE_UNUSED, + int event ATTRIBUTE_UNUSED, + void *opaque) +{ + char buffer[REMOTE_MESSAGE_MAX]; + char buffer2[4]; + struct remote_message_header hdr; + XDR xdr; + int len; + + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + + DEBUG("%s : Event fired", __FUNCTION__); + + /* Read and deserialise length word. */ + if (really_read (conn, priv, 0, buffer2, sizeof buffer2) == -1) + return; + + xdrmem_create (&xdr, buffer2, sizeof buffer2, XDR_DECODE); + if (!xdr_int (&xdr, &len)) { + error (conn, VIR_ERR_RPC, _("xdr_int (length word, reply)")); + return; + } + xdr_destroy (&xdr); + + /* Length includes length word - adjust to real length to read. */ + len -= 4; + + if (len < 0 || len > REMOTE_MESSAGE_MAX) { + error (conn, VIR_ERR_RPC, _("packet received from server too large")); + return; + } + + /* Read reply header and what follows (either a ret or an error). */ + if (really_read (conn, priv, 0, buffer, len) == -1) { + error (conn, VIR_ERR_RPC, _("error reading buffer from memory")); + return; + } + + /* Deserialise reply header. */ + xdrmem_create (&xdr, buffer, len, XDR_DECODE); + if (!xdr_remote_message_header (&xdr, &hdr)) { + error (conn, VIR_ERR_RPC, _("invalid header in event firing")); + return; + } + + if (hdr.proc == REMOTE_PROC_DOMAIN_EVENT && + hdr.direction == REMOTE_MESSAGE) { + DEBUG0("Encountered an async event"); + remoteDomainProcessEvent(conn, &xdr); + } else { + DEBUG0("invalid proc in event firing"); + error (conn, VIR_ERR_RPC, _("invalid proc in event firing")); + } +} + +void remoteDomainEventQueueFlush(int timer ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainEventPtr domEvent; + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + + DEBUG0("Flushing domain events"); + while( (domEvent = virDomainEventCallbackQueuePop(priv->domainEvents)) ) { + DEBUG(" Flushing %p", domEvent); + if(domEvent->cb) + domEvent->cb(domEvent->dom->conn, + domEvent->dom, + domEvent->event, + domEvent->opaque); Needs to also call virDomainFree(domEvent->dom) to release the object.
+ VIR_FREE(domEvent); + }
And virEventUpdateTimeout to disable the timer again.
+}
Here's a small additive patch which takes care of the timer issue diff -r 99dad81d37dd src/remote_internal.c --- a/src/remote_internal.c Sun Oct 19 13:46:36 2008 -0400 +++ b/src/remote_internal.c Sun Oct 19 14:06:33 2008 -0400 @@ -704,7 +704,7 @@ } DEBUG0("Adding Timeout for remote event queue flushing"); - if ( (priv->eventFlushTimer = virEventAddTimeout(0, + if ( (priv->eventFlushTimer = virEventAddTimeout(-1, remoteDomainEventQueueFlush, conn)) < 0) { DEBUG0("virEventAddTimeout failed: No addTimeoutImpl defined. continuing without events."); @@ -4504,6 +4504,7 @@ DEBUG0("Encountered an event while waiting for a response"); remoteDomainQueueEvent(conn, &xdr); + virEventUpdateTimeout(priv->eventFlushTimer, 0); DEBUG0("Retrying read"); xdr_destroy (&xdr); @@ -5182,4 +5183,6 @@ domEvent->opaque); VIR_FREE(domEvent); } -} + + virEventUpdateTimeout(priv->eventFlushTimer, -1); +} Regards, 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 :|
On Fri, Oct 17, 2008 at 12:02:13PM -0400, Ben Guthro wrote:
Deliver local callbacks in response to remote events
remote_internal.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 248 insertions(+), 7 deletions(-)
diff --git a/src/remote_internal.c b/src/remote_internal.c index 35b7b4b..13537f7 100644 --- a/src/remote_internal.c +++ b/src/remote_internal.c @@ -34,6 +34,7 @@ +/** remoteDomainEventFired: + * + * The callback for monitoring the remote socket + * for event data + */ +void remoteDomainEventFired(int fd ATTRIBUTE_UNUSED, + int event ATTRIBUTE_UNUSED, + void *opaque) +{ + char buffer[REMOTE_MESSAGE_MAX]; + char buffer2[4]; + struct remote_message_header hdr; + XDR xdr; + int len; + + virConnectPtr conn = opaque; + struct private_data *priv = conn->privateData; + + DEBUG("%s : Event fired", __FUNCTION__); + + /* Read and deserialise length word. */ + if (really_read (conn, priv, 0, buffer2, sizeof buffer2) == -1) + return;
Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick if (event & (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; } before we try to read any data. Regards, 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 :|
Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick
if (event & (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; }
I've been looking over the rest of your changes. Generally, I agree all these suggestions are good ones...except for the code above With this code in, I run the following test 1. start libvirtd 2. begin to monitor events with event-test 3. virsh create foo.xml At this point, the event-test app encounters a HUP, or ERR, and stops monitoring for events - it will only ever get the "Started" event I handle this in the event-test poll loop via if ( pfd.revents & POLLHUP ) { DEBUG0("Reset by peer"); return -1; } Is it not a reasonable restriction to require the client app to handle a Hangup?
On Mon, Oct 20, 2008 at 10:48:38AM -0400, Ben Guthro wrote:
Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick
if (event & (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; }
I've been looking over the rest of your changes. Generally, I agree all these suggestions are good ones...except for the code above
With this code in, I run the following test 1. start libvirtd 2. begin to monitor events with event-test 3. virsh create foo.xml
At this point, the event-test app encounters a HUP, or ERR, and stops monitoring for events - it will only ever get the "Started" event
That's a little odd - I'm not sure why 'event-test' would be getting a HUP/ERR when 'virsh' starts a domain. 'event-test' should only get a HUP/ERR if it looses its socket connection to the libvirtd server. Once you've lost the connection like this, the entire virConnectPtr is non-operative, and the client app needs to create a new virConnectPtr to re-connect. So removing the FD from the event loop shouldn't result in us loosing any events - we're already doomed there.to
I handle this in the event-test poll loop via
if ( pfd.revents & POLLHUP ) { DEBUG0("Reset by peer"); return -1; }
The integration into the event loop though is only something the app will have general control off - eg, in the example libvirt-glib code I posted its totally opaque to the app.
Is it not a reasonable restriction to require the client app to handle a Hangup?
Once the socket is dead, all subsequent libvirt call made on that virConnectPtr object will throw errors, which the app should see. Though if they're only ever waiting for events, and not making any other calls, they'd not see it. Perhaps we could do with an explicit callback for connection disconnect. 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 :|
OK - it looks like my EventImpl was passing along the wrong bits. I'll look into the "token" scheme suggested in an earlier email, and get this ready for re-submission. Would you prefer a new patch series, as before - or another patch that modifies the prior series? Daniel P. Berrange wrote on 10/20/2008 10:59 AM:
On Mon, Oct 20, 2008 at 10:48:38AM -0400, Ben Guthro wrote:
Just discovered one tiny problem here - need to check 'event' to see if the POLLHUP or POLLERR flags are set, and unregister the callback. Otherwise if you kill the server, the client will just spin on POLLHUP or ERR forever. Something like this ought todo the trick
if (event & (POLLERR | POLLHUP)) { virEventRemoveHandle(fd); return; }
I've been looking over the rest of your changes. Generally, I agree all these suggestions are good ones...except for the code above
With this code in, I run the following test 1. start libvirtd 2. begin to monitor events with event-test 3. virsh create foo.xml
At this point, the event-test app encounters a HUP, or ERR, and stops monitoring for events - it will only ever get the "Started" event
That's a little odd - I'm not sure why 'event-test' would be getting a HUP/ERR when 'virsh' starts a domain.
'event-test' should only get a HUP/ERR if it looses its socket connection to the libvirtd server. Once you've lost the connection like this, the entire virConnectPtr is non-operative, and the client app needs to create a new virConnectPtr to re-connect. So removing the FD from the event loop shouldn't result in us loosing any events - we're already doomed there.to
I handle this in the event-test poll loop via
if ( pfd.revents & POLLHUP ) { DEBUG0("Reset by peer"); return -1; }
The integration into the event loop though is only something the app will have general control off - eg, in the example libvirt-glib code I posted its totally opaque to the app.
Is it not a reasonable restriction to require the client app to handle a Hangup?
Once the socket is dead, all subsequent libvirt call made on that virConnectPtr object will throw errors, which the app should see. Though if they're only ever waiting for events, and not making any other calls, they'd not see it. Perhaps we could do with an explicit callback for connection disconnect.
Daniel
On Mon, Oct 20, 2008 at 11:43:24AM -0400, Ben Guthro wrote:
OK - it looks like my EventImpl was passing along the wrong bits.
I'll look into the "token" scheme suggested in an earlier email, and get this ready for re-submission.
Would you prefer a new patch series, as before - or another patch that modifies the prior series?
Best to just re-post the whole series with changes incorporated, rather than trying to add more patches ontop of patches. Regards, 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 :|
Register for, and dispatch domain event callbacks qemu_conf.h | 3 ++ qemu_driver.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 2 deletions(-)
On Fri, Oct 17, 2008 at 12:02:52PM -0400, Ben Guthro wrote:
Register for, and dispatch domain event callbacks
qemu_conf.h | 3 ++ qemu_driver.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/src/qemu_conf.h b/src/qemu_conf.h index cfd7d35..d06c4d7 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -63,6 +63,9 @@ struct qemud_driver { char *vncListen;
virCapsPtr caps; + + /* An array of callbacks */ + virDomainEventCallbackListPtr domainEventCallbacks; };
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index a86b787..9792541 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -159,6 +159,10 @@ qemudStartup(void) { /* Don't have a dom0 so start from 1 */ qemu_driver->nextvmid = 1;
+ /* Init callback list */ + if(VIR_ALLOC(qemu_driver->domainEventCallbacks) < 0) + return -1; + if (!uid) { if (asprintf(&qemu_driver->logDir, "%s/log/libvirt/qemu", LOCAL_STATE_DIR) == -1) @@ -301,6 +305,9 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->autostartDir); VIR_FREE(qemu_driver->vncTLSx509certdir);
+ /* Free domain callback list */ + virDomainEventCallbackListFree(qemu_driver->domainEventCallbacks); + if (qemu_driver->brctl) brShutdown(qemu_driver->brctl);
@@ -742,6 +749,8 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { return -1; }
+static virDomainPtr qemudDomainLookupByName(virConnectPtr conn, const char *name); + static int qemudStartVMDaemon(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, @@ -756,6 +765,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, unsigned int qemuCmdFlags; fd_set keepfd; const char *emulator; + virDomainPtr dom;
FD_ZERO(&keepfd);
@@ -918,6 +928,11 @@ static int qemudStartVMDaemon(virConnectPtr conn, qemudShutdownVMDaemon(conn, driver, vm); return -1; } + dom = qemudDomainLookupByName(conn,vm->def->name); + if(dom) + virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_STARTED); + else + DEBUG0("Warning - dom is NULL at domain start"); }
return ret; @@ -1503,6 +1518,7 @@ static int qemudDomainSuspend(virDomainPtr dom) { } vm->state = VIR_DOMAIN_PAUSED; qemudDebug("Reply %s", info); + virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_SUSPENDED); VIR_FREE(info); return 0; } @@ -1531,6 +1547,7 @@ static int qemudDomainResume(virDomainPtr dom) { } vm->state = VIR_DOMAIN_RUNNING; qemudDebug("Reply %s", info); + virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_RESUMED); VIR_FREE(info); return 0; } @@ -1572,7 +1589,7 @@ static int qemudDomainDestroy(virDomainPtr dom) { if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); - + virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_STOPPED); return 0; }
@@ -1903,7 +1920,7 @@ static int qemudDomainSave(virDomainPtr dom, if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); - + virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_SAVED); return 0; }
@@ -2104,6 +2121,7 @@ static int qemudDomainRestore(virConnectPtr conn, struct qemud_driver *driver = (struct qemud_driver *)conn->privateData; virDomainDefPtr def; virDomainObjPtr vm; + virDomainPtr dom; int fd; int ret; char *xml; @@ -2210,6 +2228,11 @@ static int qemudDomainRestore(virConnectPtr conn, vm->state = VIR_DOMAIN_RUNNING; }
+ dom = virDomainLookupByID(conn, def->id); + if(dom) { + virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_RESTORED); + VIR_FREE(dom); + } return 0; }
@@ -3051,6 +3074,52 @@ done: }
+static int qemudDomainEventRegister (virConnectPtr conn, + void *callback, + void *opaque) +{ + struct qemud_driver *driver = (struct qemud_driver *)conn->privateData; + + return virDomainEventCallbackListAdd(conn, driver->domainEventCallbacks, callback, opaque); +} + +static int qemudDomainEventDeregister (virConnectPtr conn, + void *callback) +{ + struct qemud_driver *driver = (struct qemud_driver *)conn->privateData; + + return virDomainEventCallbackListRemove(conn, driver->domainEventCallbacks, callback); +} + +static void qemudDomainEventDispatch (virDomainPtr dom, + virDomainEventType evt) +{ + int i; + struct qemud_driver *driver; + virDomainEventCallbackListPtr cbList; + + if(!dom->conn) { + DEBUG0("Invalid conn"); + return; + } + driver = (struct qemud_driver *)dom->conn->privateData; + + if(!driver) { + DEBUG0("Invalid driver"); + return; + } + cbList = driver->domainEventCallbacks; + + for(i=0;i<cbList->count;i++) { + if(cbList->callbacks[i] && cbList->callbacks[i]->cb) + DEBUG("Dispatching callback %p %p event %d", cbList->callbacks[i], cbList->callbacks[i]->cb, evt); + cbList->callbacks[i]->cb(cbList->callbacks[i]->conn, + dom, evt, + cbList->callbacks[i]->opaque); + } + +}
THis all basically works fine, but there's one tiny missing bit. If a domain shuts down from an external trigger - eg guest admin does a 'shutdown -h', or host admin does 'kill -TERM $qemu', then no STOPPED event is fired. For this we need to hook into the qemudDispatchVMFailure() method. This introduces a small problem though - we don't have a 'virDomainPtr' object here, so we can't call the generic virDispatchDomainEvent() method. Of course virDispatchDomainEvent() just delegates back into the driver - in this case qemudDomainEventDispatch(), which has no hard requirement to have a 'virDomainPtr' object. Each registered callback has a 'virConnectPtr' object associated with it, so we can simply fetch a virDomainPtr as required. To demonstrate this I'm attching a patch which applies ontop of yours to make this method call directly. With this, I think we can remove the 'domainEventDispatch' field in driver.h - though obviously the other non-QEMU drivers need changing to match too. diff --git a/src/qemu_driver.c b/src/qemu_driver.c --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -105,6 +105,10 @@ static int qemudSetNonBlock(int fd) { return -1; } + +static void qemudDomainEventDispatch (struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainEventType evt); static void qemudDispatchVMEvent(int fd, int events, void *opaque); static int qemudStartVMDaemon(virConnectPtr conn, @@ -930,7 +934,7 @@ static int qemudStartVMDaemon(virConnect } dom = qemudDomainLookupByName(conn,vm->def->name); if(dom) - virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_STARTED); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_STARTED); else DEBUG0("Warning - dom is NULL at domain start"); } @@ -1027,6 +1031,7 @@ static int qemudDispatchVMFailure(struct static int qemudDispatchVMFailure(struct qemud_driver *driver, virDomainObjPtr vm, int fd ATTRIBUTE_UNUSED) { qemudShutdownVMDaemon(NULL, driver, vm); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_STOPPED); if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); @@ -1518,7 +1523,7 @@ static int qemudDomainSuspend(virDomainP } vm->state = VIR_DOMAIN_PAUSED; qemudDebug("Reply %s", info); - virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_SUSPENDED); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_SUSPENDED); VIR_FREE(info); return 0; } @@ -1547,7 +1552,7 @@ static int qemudDomainResume(virDomainPt } vm->state = VIR_DOMAIN_RUNNING; qemudDebug("Reply %s", info); - virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_RESUMED); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_RESUMED); VIR_FREE(info); return 0; } @@ -1586,10 +1591,10 @@ static int qemudDomainDestroy(virDomainP } qemudShutdownVMDaemon(dom->conn, driver, vm); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_STOPPED); if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); - virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_STOPPED); return 0; } @@ -1920,7 +1925,7 @@ static int qemudDomainSave(virDomainPtr if (!vm->persistent) virDomainRemoveInactive(&driver->domains, vm); - virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_SAVED); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_SAVED); return 0; } @@ -2230,7 +2235,7 @@ static int qemudDomainRestore(virConnect dom = virDomainLookupByID(conn, def->id); if(dom) { - virDispatchDomainEvent(dom, VIR_DOMAIN_EVENT_RESTORED); + qemudDomainEventDispatch(driver, vm, VIR_DOMAIN_EVENT_RESTORED); VIR_FREE(dom); } return 0; @@ -3090,31 +3095,28 @@ static int qemudDomainEventDeregister (v return virDomainEventCallbackListRemove(conn, driver->domainEventCallbacks, callback); } -static void qemudDomainEventDispatch (virDomainPtr dom, +static void qemudDomainEventDispatch (struct qemud_driver *driver, + virDomainObjPtr vm, virDomainEventType evt) { int i; - struct qemud_driver *driver; virDomainEventCallbackListPtr cbList; - if(!dom->conn) { - DEBUG0("Invalid conn"); - return; - } - driver = (struct qemud_driver *)dom->conn->privateData; - - if(!driver) { - DEBUG0("Invalid driver"); - return; - } cbList = driver->domainEventCallbacks; for(i=0;i<cbList->count;i++) { - if(cbList->callbacks[i] && cbList->callbacks[i]->cb) - DEBUG("Dispatching callback %p %p event %d", cbList->callbacks[i], cbList->callbacks[i]->cb, evt); - cbList->callbacks[i]->cb(cbList->callbacks[i]->conn, - dom, evt, - cbList->callbacks[i]->opaque); + if(cbList->callbacks[i] && cbList->callbacks[i]->cb) { + virConnectPtr conn = cbList->callbacks[i]->conn; + virDomainPtr dom = virGetDomain(conn, vm->def->name, vm->def->uuid); + if (dom) { + dom->id = virDomainIsActive(vm) ? vm->def->id : -1; + DEBUG("Dispatching callback %p %p event %d", cbList->callbacks[i], cbList->callbacks[i]->cb, evt); + cbList->callbacks[i]->cb(cbList->callbacks[i]->conn, + dom, evt, + cbList->callbacks[i]->opaque); + virDomainFree(dom); + } + } } } @@ -3191,7 +3193,7 @@ static virDriver qemuDriver = { #endif qemudDomainEventRegister, /* domainEventRegister */ qemudDomainEventDeregister, /* domainEventDeregister */ - qemudDomainEventDispatch, /* domainEventDispatch */ + NULL, }; Regards, 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 :|
Minor changes to LXC driver structure to prevent compile errors lxc_driver.c | 3 +++ 1 file changed, 3 insertions(+)
Minor changes to openvz driver structure to prevent compile errors openvz_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/openvz_driver.c b/src/openvz_driver.c index b82d0df..56dfea1 100644 --- a/src/openvz_driver.c +++ b/src/openvz_driver.c @@ -1012,6 +1012,9 @@ static virDriver openvzDriver = { NULL, /* domainMemoryPeek */ NULL, /* nodeGetCellsFreeMemory */ NULL, /* nodeGetFreeMemory */ + NULL, /* domainEventRegister */ + NULL, /* domainEventDeregister */ + NULL, /* domainEventDispatch */ }; int openvzRegister(void) {
Minor changes to test driver structure to prevent compile errors test.c | 3 +++ 1 file changed, 3 insertions(+)
Test app, and infrastructure changes for an example dir Makefile.am | 2 configure.in | 3 examples/domain-events/events-c/Makefile.am | 5 examples/domain-events/events-c/event-test.c | 214 +++++++++++++++++++++++++++ libvirt.spec.in | 3 5 files changed, 225 insertions(+), 2 deletions(-)
Add functions to be ignored, to prevent rpm build failures generator.py | 3 +++ 1 file changed, 3 insertions(+)
On Fri, Oct 17, 2008 at 11:51:51AM -0400, Ben Guthro wrote:
The following patch series implements the Events API discussed here previously in this thread: http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html
By Daniel B's advice - I have broken this patch into the following:
Thanks that has made it very nice to review ! In a quick 5 minute read through, I don't see any major issues that stand out to me - architecturally I looks pretty much exactly as I had imagined it would. I'll do a more detailed review over the weekend so there's some feedback for monday. Regards, 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 :|
Ben Guthro <bguthro@virtualiron.com> wrote:
The following patch series implements the Events API discussed here previously in this thread: http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html ...
Nice work. I spotted no problems in review, and noticed only one when testing: In the new file, examples/domain-events/events-c/Makefile.am
diff --git a/examples/domain-events/events-c/Makefile.am b/examples/domain-events/events-c/Makefile.am ... @@ -0,0 +1,5 @@ +INCLUDES = -I@top_srcdir@/include +bin_PROGRAMS = event-test +event_test_CFLAGS = $(WARN_CFLAGS) +event_test_SOURCES = event-test.c +event_test_LDADD = -L@top_srcdir@/src/.libs -lvirt
that would fail with a non-srcdir build (i.e., when the build_dir != srcdir), because then @top_srcdir@/src/.libs doesn't exist, and that causes "make distcheck" to fail. Also, it is best not to reference libtool's .libs/ directories directly, so you can use this instead: diff --git a/examples/domain-events/events-c/Makefile.am b/examples/domain-events/events-c/Makefile.am index 1788dac..3c63ca3 100644 --- a/examples/domain-events/events-c/Makefile.am +++ b/examples/domain-events/events-c/Makefile.am @@ -2,4 +2,4 @@ INCLUDES = -I@top_srcdir@/include bin_PROGRAMS = event-test event_test_CFLAGS = $(WARN_CFLAGS) event_test_SOURCES = event-test.c -event_test_LDADD = -L@top_srcdir@/src/.libs -lvirt +event_test_LDADD = @top_builddir@/src/libvirt.la Finally, I noticed that "make check" does not run the new event-test program that's built there. Should it?
Great to hear this all works for you. I'll be sure to take that automake change (autoconf/automake is still a bit new to me, and I still find it a bit cryptic) As for the test app - I don't think that it is appropriate to run from make check It was more designed to be a developer example, as suggested here: https://www.redhat.com/archives/libvir-list/2008-October/msg00258.html -----Original Message----- From: Jim Meyering [mailto:jim@meyering.net] Sent: Sun 10/19/2008 6:23 AM To: Ben Guthro Cc: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH 00/12] Domain Events - Round 2 Ben Guthro <bguthro@virtualiron.com> wrote:
The following patch series implements the Events API discussed here previously in this thread: http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html ...
Nice work. I spotted no problems in review, and noticed only one when testing: In the new file, examples/domain-events/events-c/Makefile.am
diff --git a/examples/domain-events/events-c/Makefile.am b/examples/domain-events/events-c/Makefile.am ... @@ -0,0 +1,5 @@ +INCLUDES = -I@top_srcdir@/include +bin_PROGRAMS = event-test +event_test_CFLAGS = $(WARN_CFLAGS) +event_test_SOURCES = event-test.c +event_test_LDADD = -L@top_srcdir@/src/.libs -lvirt
that would fail with a non-srcdir build (i.e., when the build_dir != srcdir), because then @top_srcdir@/src/.libs doesn't exist, and that causes "make distcheck" to fail. Also, it is best not to reference libtool's .libs/ directories directly, so you can use this instead: diff --git a/examples/domain-events/events-c/Makefile.am b/examples/domain-events/events-c/Makefile.am index 1788dac..3c63ca3 100644 --- a/examples/domain-events/events-c/Makefile.am +++ b/examples/domain-events/events-c/Makefile.am @@ -2,4 +2,4 @@ INCLUDES = -I@top_srcdir@/include bin_PROGRAMS = event-test event_test_CFLAGS = $(WARN_CFLAGS) event_test_SOURCES = event-test.c -event_test_LDADD = -L@top_srcdir@/src/.libs -lvirt +event_test_LDADD = @top_builddir@/src/libvirt.la Finally, I noticed that "make check" does not run the new event-test program that's built there. Should it?
On Fri, Oct 17, 2008 at 11:51:51AM -0400, Ben Guthro wrote:
The following patch series implements the Events API discussed here previously in this thread: http://www.redhat.com/archives/libvir-list/2008-September/msg00321.html and http://www.redhat.com/archives/libvir-list/2008-October/msg00245.html
By Daniel B's advice - I have broken this patch into the following:
events-01-public-api Changes to libvirt.h, and libvirt.c events-02-internal-api Changes to internal.h and event code events-03-qemud Changes to the daemon events-04-qemud-rpc-protocol Changes to the qemud rpc protocol events-05-driver.patch Changes to the driver API events-06-driver-qemu Changes to the qemu driver events-07-driver-remote Changes to the remote driver events-08-driver-lxc Minor changes to LXC driver structure to prevent compile errors events-09-driver-openvz Minor changes to openvz driver structure to prevent compile errors events-10-driver-test Minor changes to test driver structure to prevent compile errors events-11-example-testapp Test app, and infrastructure changes for an example dir events-12-python-ignore.patch Add functions to be ignored, for now
For now, domain events are only emitted from qemu/kvm guests.
I was interested to see how well we could integrate with glib, so I knocked up a minimal 'libvirt-glib' library, which provides an event loop which calls into glib. I also adapted your demo program to use this too. The code is in this mercurial repository http://hg.berrange.com/libraries/libvirt-glib--devel This is where I spotted the timeout bug I mentioned earlier - aside from that it works fairly nicely. 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 (5)
-
Ben Guthro -
Daniel P. Berrange -
Daniel Veillard -
David Lively -
Jim Meyering