On Thu, Oct 09, 2008 at 11:22:38AM -0400, Ben Guthro wrote: For large patches like this, its useful to split it up in order of: - The public header API & libvirt.c/driver.c glue layer - The daemon code - One patch per internal driver I hope the review that follows doesn't come off too negative - its basically all just hinging around one core point. Namely that the tracking & dispatch of callbacks needs to be pushed further down into the drivers themselves. This is to avoid the driver having to keep so many references back to the public facing objects & state which rather tangles things up. We probably want to explicit define these constants in our header file, since now this is public we need it to be portable, regardless of the internal impl. I'm not sure why the daemon needs to know this. I'd expect that it just gets a RPC call for each virConnectDomainEventRegister() and virConnectDomainEventDeregister() invocation, and pass these straight into the 'virConnectPtr' it has, not register one global callback and try to filter events itself. This check shoudln't be needed if you're only registering the callbacks when the client actually asks for them. Hence this shouldn't be needed at time of open. This is where we should be registering the callbacks. And equivalent RPC handlers for unregistering. This also doesn't take into account that a single virConnectPtr object could have multiple callbacks registered to it. eg, if you register 2 callbacks, and then unregister one, the 2nd will no longer get events either. Rather than having an enable flag, the wire protocol should mirror the public API, with register & unregister RPC calls. The success flag is redundant - we always serialize an explicit error reply if something fails. Likewise the internal driver API shoudl mirror the public API pretty much 1-for-1, rather than multiplexing multiple public APIs onto 1 private API. The static driver callback records should not need to know this information - in fact they should be declared 'const' Any registered callbacks should be tracked internally to the drivers - if applicable. In the Xen driver there's no need to track all virConnect objects because each virConnect instance will explicitly be getting events from xenstored independantly of any other virConnect object. For stateful drivers, like QEMU, the callbacks should eb tracked in the internal driver state - eg 'struct qemud_driver' or the equivalent. There's no need to keep around this API now we've split it up - it was an internal only symbol, so just update the libvirtd code to call to the new APIs instead. This doesn't make any sense to me - you're making all the virConnect objects cross reference each other. Each object only needs to know about the callbacks registered to itself - it doesn't care about callbacks other virConnect objects have. I'd expect something more like struct _virDomainEventCallback { virConnectDomainEventCallback cb; void *opaque; }; typedef struct _virDomainEventCallback virDomainEventCallback; typedef virDomainEventCallback *virDomainEventCallbackPtr; struct _virDomainEventCallbackList { unsigned int ndomainCallbacks; struct __virDomainEventCallbackPtr *domainCallbacks; } typedef struct _virDomainEventCallbackList virDomainEventCallbackList; I used a explicit array here, because we're trying to kill off the linked lists, since it simplifies thread safety work, to be able to lock individual objects, and not worry about its peers pointing to it - only its parent. The internal drivers would then have virDomainEventCallbackList intances as they need - eg, in the _xenUnifiedPrivate struct for Xen drivers, or in the 'struct qemud_driver' for QEMU, etc. The virDriver tables should be considered const & not changed. This and all the other chunks doing the same thing are not required. When you call down into the internal driver's 'domainDestroy' method it should take care of raising events (if required). In the case of Xen, there'll be no need for the driver itself to ever raise events, because these get raised indirectly by the xenstore watches. In the case of the QEMU driver (and other stateful drivers), the intenral driver state (eg, struct qemud_driver) should have a list of all active callbacks, and will invoke them when domains start/die/pause etc. So explicitly raising events in the libvirt.c file is unneccessary, and can actually result in bogus and/or missed events. eg if you send SIGHUP to the daemon it will reload QEMU config files and autostart guests. None of this goes via the libvirt.c file, so if we were to rely on libvirt.c for raising events that would imply it would miss many events. Or in the domain destroy example - an admin can destroy a domain outside of libvirt by simply sending kill -TERM to the QEMU process. So again, the qemu driver itself must take responsbility for raising the events. I see further down you've already got the QEMU driver raising events when needed, so I imagine you can just delete all this stuff in libvirt.c The callbacks should be passed right down into the drivers for them to track as they need. And the internal method naming should map to the public API naming. The QEMU driver needs to be tracking the callbacks against its 'struct qemud_driver' struct explicitly - the virDriver should be considered readonly. Not sure I see where this is exposed / used in the public API ? There's a small safety problem here - it is not re-entrant safe. eg the app could be in a virDomainCreate() call, get an event saying that a domain has paused, invoke its callback, and the callback then calls into another libvirt API, ad-infinitum. Aside from the mutex locks here in the call() method, this is also cause problems with locks in the application itself which we can't rely on being recursive mutexes. So for sake of safety, we need to queue up all events we receive while reading a RPC reply. Then at the end of the call() method, register a timeout event with 0 second timeout. So when the current libvirt API call completes, and the application goes back into its event loop, the timeout will immediately fire, and we can dispatch the queued libvirt events from the known safe context. In fat I think this scheme ought to let you do away with the mutex locking completely. The contract for virConnectPtr dictates that you are forbidden to use a single virConnectPtr object from more than one thread at once, so if we're queueing & dispatching events from and timeout handler, we shouldn't ever get a reentrancy/locking problem. 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 :|