10:51 a.m.
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.
10:53 a.m.
New subject: [libvirt] Re: [PATCH 01/12] Domain Events - Public API
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(-)
2:10 p.m.
New subject: [libvirt] Re: [PATCH 01/12] Domain Events - Public API
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 :|
10:04 a.m.
New subject: [libvirt] Re: [PATCH 01/12] Domain Events - Public API
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:56 a.m.
New subject: [libvirt] Re: [PATCH 03/12] Domain Events - daemon changes
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(-)
10:56 a.m.
New subject: [libvirt] Re: [PATCH 02/12] Domain Events - Internal API
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(-)
2:18 p.m.
New subject: [libvirt] Re: [PATCH 02/12] Domain Events - Internal API
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 :|
10:58 a.m.
New subject: [libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
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(-)
2:22 p.m.
New subject: [libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
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 :|
3:05 p.m.
New subject: [libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
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
7:57 a.m.
New subject: [libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
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
8:51 a.m.
New subject: [libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
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 :|
11 a.m.
New subject: [libvirt] Re: [PATCH 05/12] Domain Events - Driver API
Additions to the driver API
driver.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
2:23 p.m.
New subject: [libvirt] Re: [PATCH 05/12] Domain Events - Driver API
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 :|
11:02 a.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
Deliver local callbacks in response to remote events
remote_internal.c | 255 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 248 insertions(+), 7 deletions(-)
2:28 p.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
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 :|
5:43 a.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
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 :|
9:48 a.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
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?
9:59 a.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
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 :|
10:43 a.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
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
10:45 a.m.
New subject: [libvirt] Re: [PATCH 07/12] Domain Events - remote driver
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 :|
11:02 a.m.
New subject: [libvirt] Re: [PATCH 06/12] Domain Events - qemu driver
Register for, and dispatch domain event callbacks
qemu_conf.h | 3 ++
qemu_driver.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 77 insertions(+), 2 deletions(-)
2:35 p.m.
New subject: [libvirt] Re: [PATCH 06/12] Domain Events - qemu driver
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 :|
11:05 a.m.
New subject: [libvirt] Re: [PATCH 08/12] Domain Events - lxc driver
Minor changes to LXC driver structure to prevent compile errors
lxc_driver.c | 3 +++
1 file changed, 3 insertions(+)
11:06 a.m.
New subject: [libvirt] Re: [PATCH 09/12] Domain Events - openvz driver
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) {
11:07 a.m.
New subject: [libvirt] Re: [PATCH 10/12] Domain Events - test driver
Minor changes to test driver structure to prevent compile errors
test.c | 3 +++
1 file changed, 3 insertions(+)
11:08 a.m.
New subject: [libvirt] Re: [PATCH 11/12] Domain Events - test harness
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(-)
11:09 a.m.
New subject: [libvirt] Re: [PATCH 12/12] Domain Events - python
Add functions to be ignored, to prevent rpm build failures
generator.py | 3 +++
1 file changed, 3 insertions(+)
11:24 a.m.
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 :|
5:23 a.m.
Ben Guthro <bguthro(a)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(a)/src/.libs -lvirt
that would fail with a non-srcdir build (i.e., when the
build_dir != srcdir), because then @top_srcdir(a)/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(a)/src/.libs -lvirt
+event_test_LDADD = @top_builddir(a)/src/libvirt.la
Finally, I noticed that "make check" does not run the new
event-test program that's built there. Should it?
8:10 a.m.
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(a)redhat.com
Subject: Re: [libvirt] [PATCH 00/12] Domain Events - Round 2
Ben Guthro <bguthro(a)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(a)/src/.libs -lvirt
that would fail with a non-srcdir build (i.e., when the
build_dir != srcdir), because then @top_srcdir(a)/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(a)/src/.libs -lvirt
+event_test_LDADD = @top_builddir(a)/src/libvirt.la
Finally, I noticed that "make check" does not run the new
event-test program that's built there. Should it?
2:37 p.m.
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 :|
5876
days inactive
5880
days old
31 comments
5 participants
participants (5)
-
Ben Guthro -
Daniel P. Berrange -
Daniel Veillard -
David Lively -
Jim Meyering