Re: [libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
On Tue, Oct 21, 2008 at 03:14:49PM -0400, Ben Guthro wrote:
[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 | 29 +
remote_protocol.h | 25 +
remote_protocol.x | 25 -
6 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/qemud/remote_protocol.x b/qemud/remote_protocol.x
index f1bd9ff..b7e41aa 100644
--- a/qemud/remote_protocol.x
+++ b/qemud/remote_protocol.x
@@ -965,6 +965,25 @@ struct remote_storage_vol_get_path_ret {
remote_nonnull_string name;
};
+/**
+ * Events Register/Deregister:
+ * It would seem rpcgen does not like both args, and ret
+ * to be null. It will not generate the prototype otherwise.
+ * Pass back a redundant boolean to force prototype generation.
+ */
Oh yes, I'd come across this problem in the past too. Adding
the cb_registerd field is harmless enough, so lets do it.
+struct remote_domain_events_register_ret {
+int cb_registered;
+};
+
+struct remote_domain_events_deregister_ret {
+int cb_registered;
+};
+
+struct remote_domain_event_ret {
+remote_nonnull_domain dom;
+int event;
+};
+
/*- Protocol. -*/
/* Define the program number, protocol version and procedure numbers here. */
@@ -1086,7 +1105,11 @@ enum remote_procedure {
REMOTE_PROC_NODE_GET_FREE_MEMORY = 102,
REMOTE_PROC_DOMAIN_BLOCK_PEEK = 103,
-REMOTE_PROC_DOMAIN_MEMORY_PEEK = 104
+REMOTE_PROC_DOMAIN_MEMORY_PEEK = 104,
+
+REMOTE_PROC_DOMAIN_EVENTS_REGISTER = 105,
+REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER = 106,
+REMOTE_PROC_DOMAIN_EVENT = 107
};
ACK to this 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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Re: [PATCH 04/12] Domain Events - rpc changes
[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 | 29 + remote_protocol.h | 25 + remote_protocol.x | 25 - 6 files changed, 102 insertions(+), 1 deletion(-) diff --git a/qemud/remote_dispatch_localvars.h b/qemud/remote_dispatch_localvars.h index f46b493..505fb30 100644 --- a/qemud/remote_dispatch_localvars.h +++ b/qemud/remote_dispatch_localvars.h @@ -6,6 +6,7 @@ remote_domain_lookup_by_uuid_args lv_remote_domain_lookup_by_uuid_args; remote_domain_lookup_by_uuid_ret lv_remote_domain_lookup_by_uuid_ret; remote_storage_pool_list_volumes_args lv_remote_storage_pool_list_volumes_args; remote_storage_pool_list_volumes_ret lv_remote_storage_pool_list_volumes_ret; +remote_domain_events_deregister_ret lv_remote_domain_events_deregister_ret; remote_domain_shutdown_args lv_remote_domain_shutdown_args; remote_list_defined_domains_args lv_remote_list_defined_domains_args; remote_list_defined_domains_ret lv_remote_list_defined_domains_ret; @@ -20,6 +21,7 @@ remote_domain_get_autostart_args lv_remote_domain_get_autostart_args; remote_domain_get_autostart_ret lv_remote_domain_get_autostart_ret; remote_domain_set_vcpus_args lv_remote_domain_set_vcpus_args; remote_get_hostname_ret lv_remote_get_hostname_ret; +remote_domain_events_register_ret lv_remote_domain_events_register_ret; remote_network_undefine_args lv_remote_network_undefine_args; remote_domain_create_args lv_remote_domain_create_args; remote_network_destroy_args lv_remote_network_destroy_args; @@ -121,6 +123,7 @@ remote_num_of_defined_storage_pools_ret lv_remote_num_of_defined_storage_pools_r remote_domain_core_dump_args lv_remote_domain_core_dump_args; remote_list_defined_storage_pools_args lv_remote_list_defined_storage_pools_args; remote_list_defined_storage_pools_ret lv_remote_list_defined_storage_pools_ret; +remote_domain_event_ret lv_remote_domain_event_ret; remote_domain_get_max_memory_args lv_remote_domain_get_max_memory_args; remote_domain_get_max_memory_ret lv_remote_domain_get_max_memory_ret; remote_num_of_domains_ret lv_remote_num_of_domains_ret; diff --git a/qemud/remote_dispatch_proc_switch.h b/qemud/remote_dispatch_proc_switch.h index 89296d7..3cd5d78 100644 --- a/qemud/remote_dispatch_proc_switch.h +++ b/qemud/remote_dispatch_proc_switch.h @@ -116,6 +116,24 @@ case REMOTE_PROC_DOMAIN_DUMP_XML: ret = (char *) lv_remote_domain_dump_xml_ret; memset (lv_remote_domain_dump_xml_ret, 0, sizeof lv_remote_domain_dump_xml_ret); break; +case REMOTE_PROC_DOMAIN_EVENT: +fn = (dispatch_fn) remoteDispatchDomainEvent; +ret_filter = (xdrproc_t) xdr_remote_domain_event_ret; +ret = (char *) lv_remote_domain_event_ret; +memset (lv_remote_domain_event_ret, 0, sizeof lv_remote_domain_event_ret); +break; +case REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER: +fn = (dispatch_fn) remoteDispatchDomainEventsDeregister; +ret_filter = (xdrproc_t) xdr_remote_domain_events_deregister_ret; +ret = (char *) lv_remote_domain_events_deregister_ret; +memset (lv_remote_domain_events_deregister_ret, 0, sizeof lv_remote_domain_events_deregister_ret); +break; +case REMOTE_PROC_DOMAIN_EVENTS_REGISTER: +fn = (dispatch_fn) remoteDispatchDomainEventsRegister; +ret_filter = (xdrproc_t) xdr_remote_domain_events_register_ret; +ret = (char *) lv_remote_domain_events_register_ret; +memset (lv_remote_domain_events_register_ret, 0, sizeof lv_remote_domain_events_register_ret); +break; case REMOTE_PROC_DOMAIN_GET_AUTOSTART: fn = (dispatch_fn) remoteDispatchDomainGetAutostart; args_filter = (xdrproc_t) xdr_remote_domain_get_autostart_args; diff --git a/qemud/remote_dispatch_prototypes.h b/qemud/remote_dispatch_prototypes.h index 3f4eb9f..4b27a9f 100644 --- a/qemud/remote_dispatch_prototypes.h +++ b/qemud/remote_dispatch_prototypes.h @@ -18,6 +18,9 @@ static int remoteDispatchDomainDefineXml (struct qemud_server *server, struct qe static int remoteDispatchDomainDestroy (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, remote_domain_destroy_args *args, void *ret); static int remoteDispatchDomainDetachDevice (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, remote_domain_detach_device_args *args, void *ret); static int remoteDispatchDomainDumpXml (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, remote_domain_dump_xml_args *args, remote_domain_dump_xml_ret *ret); +static int remoteDispatchDomainEvent (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, void *args, remote_domain_event_ret
Re: [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
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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(-) diff --git a/qemud/remote_dispatch_localvars.h b/qemud/remote_dispatch_localvars.h index f46b493..df36239 100644 --- a/qemud/remote_dispatch_localvars.h +++ b/qemud/remote_dispatch_localvars.h @@ -6,6 +6,7 @@ remote_domain_lookup_by_uuid_args lv_remote_domain_lookup_by_uuid_args; remote_domain_lookup_by_uuid_ret lv_remote_domain_lookup_by_uuid_ret; remote_storage_pool_list_volumes_args lv_remote_storage_pool_list_volumes_args; remote_storage_pool_list_volumes_ret lv_remote_storage_pool_list_volumes_ret; +remote_domain_events_deregister_args lv_remote_domain_events_deregister_args; remote_domain_shutdown_args lv_remote_domain_shutdown_args; remote_list_defined_domains_args lv_remote_list_defined_domains_args; remote_list_defined_domains_ret lv_remote_list_defined_domains_ret; @@ -20,6 +21,7 @@ remote_domain_get_autostart_args lv_remote_domain_get_autostart_args; remote_domain_get_autostart_ret lv_remote_domain_get_autostart_ret; remote_domain_set_vcpus_args lv_remote_domain_set_vcpus_args; remote_get_hostname_ret lv_remote_get_hostname_ret; +remote_domain_events_register_args lv_remote_domain_events_register_args; remote_network_undefine_args lv_remote_network_undefine_args; remote_domain_create_args lv_remote_domain_create_args; remote_network_destroy_args lv_remote_network_destroy_args; @@ -121,6 +123,7 @@ remote_num_of_defined_storage_pools_ret lv_remote_num_of_defined_storage_pools_r remote_domain_core_dump_args lv_remote_domain_core_dump_args; remote_list_defined_storage_pools_args lv_remote_list_defined_storage_pools_args; remote_list_defined_storage_pools_ret lv_remote_list_defined_storage_pools_ret; +remote_domain_event_ret lv_remote_domain_event_ret; remote_domain_get_max_memory_args lv_remote_domain_get_max_memory_args; remote_domain_get_max_memory_ret lv_remote_domain_get_max_memory_ret; remote_num_of_domains_ret lv_remote_num_of_domains_ret; diff --git a/qemud/remote_dispatch_proc_switch.h b/qemud/remote_dispatch_proc_switch.h index 89296d7..2254a96 100644 --- a/qemud/remote_dispatch_proc_switch.h +++ b/qemud/remote_dispatch_proc_switch.h @@ -116,6 +116,24 @@ case REMOTE_PROC_DOMAIN_DUMP_XML: ret = (char *) lv_remote_domain_dump_xml_ret; memset (lv_remote_domain_dump_xml_ret, 0, sizeof lv_remote_domain_dump_xml_ret); break; +case REMOTE_PROC_DOMAIN_EVENT: +fn = (dispatch_fn) remoteDispatchDomainEvent; +ret_filter = (xdrproc_t) xdr_remote_domain_event_ret; +ret = (char *) lv_remote_domain_event_ret; +memset (lv_remote_domain_event_ret, 0, sizeof lv_remote_domain_event_ret); +break; +case REMOTE_PROC_DOMAIN_EVENTS_DEREGISTER: +fn = (dispatch_fn) remoteDispatchDomainEventsDeregister; +args_filter = (xdrproc_t) xdr_remote_domain_events_deregister_args; +args = (char *) lv_remote_domain_events_deregister_args; +memset (lv_remote_domain_events_deregister_args, 0, sizeof lv_remote_domain_events_deregister_args); +break; +case REMOTE_PROC_DOMAIN_EVENTS_REGISTER: +fn = (dispatch_fn) remoteDispatchDomainEventsRegister; +args_filter = (xdrproc_t) xdr_remote_domain_events_register_args; +args = (char *) lv_remote_domain_events_register_args; +memset (lv_remote_domain_events_register_args, 0, sizeof lv_remote_domain_events_register_args); +break; case REMOTE_PROC_DOMAIN_GET_AUTOSTART: fn = (dispatch_fn) remoteDispatchDomainGetAutostart; args_filter = (xdrproc_t) xdr_remote_domain_get_autostart_args; diff --git a/qemud/remote_dispatch_prototypes.h b/qemud/remote_dispatch_prototypes.h index 3f4eb9f..79e26e3 100644 --- a/qemud/remote_dispatch_prototypes.h +++ b/qemud/remote_dispatch_prototypes.h @@ -18,6 +18,9 @@ static int remoteDispatchDomainDefineXml (struct qemud_server *server, struct qe static int remoteDispatchDomainDestroy (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, remote_domain_destroy_args *args, void *ret); static int remoteDispatchDomainDetachDevice (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, remote_domain_detach_device_args *args, void *ret); static int remoteDispatchDomainDumpXml (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, remote_domain_dump_xml_args *args, remote_domain_dump_xml_ret *ret); +static int remoteDispatchDomainEvent (struct qemud_server *server, struct qemud_client *client, remote_message_header *req, void *args, remote_domain_event_ret *ret); +static int
