[libvirt] Re: [PATCH 04/12] Domain Events - rpc changes

[David Lively]

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