[libvirt] [RFC] making (newly public) EventImpl interface moreconsistent

[Daniel P. Berrange]

On Wed, Nov 05, 2008 at 11:26:07AM -0500, David Lively wrote:
> On Wed, 2008-11-05 at 11:51 +0000, Daniel P. Berrange wrote:
> > DevKit & HAL are just APIs built ontop of DBus, so the key here
> > is integration with DBus watch APIs. AFAIK, those only require
> > that the event loop impl have one callback per unique FD. 
> 
> Here's what I'm seeing when registering for dbus watch callbacks.  In
> halNodeDriverStartup (in node_device_hal.c in the submitted host dev
> enum patch), I register for dbus watch callbacks:
> 
>     /* Register dbus watch callbacks */
>     if (!dbus_connection_set_watch_functions(dbus_conn,
>                                              add_dbus_watch,
>                                              remove_dbus_watch,
>                                              toggle_dbus_watch,
>                                              NULL, NULL)) {
>         fprintf(stderr, "%s: dbus_connection_set_watch_functions failed\n",
>                 __FUNCTION__);
>         goto failure;
>     }
> 
> And then I instrumented add/remove/toggle_dbus_watch.  add_dbus_watch is
> called twice as soon as we register the watch functions:
>   add_dbus_watch 0xae4200  fd 6  flags 0x2  enabled 0
>   add_dbus_watch 0xaeb950  fd 6  flags 0x1  enabled 1
>   *** DUPLICATE HANDLE 6 at [0] ***
> So here we have two different DBusWatches sharing the same unix fd.  In
> this case, the first one (POLLOUT flags) is disabled, and never toggled,
> so things happen to work just fine.  The current qemud AddHandleImpl
> will in fact overwrite the first entry with the second, so it has
> totally lost the first watch.
> 
> But this is just lucky.  Because the behavior of adding a duplicate
> handle is undefined, the implementation could just as well have ignored
> the second entry, in which case DBus events would never be received.
> 
> I'll look into the glib handle-watching code (which I'm not familiar
> with currently) and see how it behaves, but I can't imagine it doesn't
> support this when DBus watches clearly require it.

I think the problem here is that the existing Avahi mdns code in the
libvirtd daemon is also already using DBus, and has already setup the
DBus system bus instances to integrate with the libvirtd event loop.

By default, there is a single DBus system bus connection in the app
which is shared amongst all users. The DBus API spec for its watch 
functions mandates that the application only setup watches once per
connection, so having both the Avahi and HAL/DevKit code in libvirt 
call dbus_connection_set_watch_functions() against the shared connection
is in fact a bug. For added fun, PolicyKit code in the daemon also makes
use of DBus, but doesn't (yet) need callbacks so hasn't done mainloop
integration.

So the flaw here is that the individual drivers should not all be
attempting to setup integration with the shared dbus system bus
connection. Either each driver should use a private dbus system bus
conenction, or the main daemon startup code should take responsibility
for integrating the shared connection instance into its main loop.

I'd be inclined to go for the latter. For simplicitly, I think you
ought to simply be able to remove the dbus_connection_set_watch_functions
call from the driver, and rely on fact that Avahi code has already done
this.

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 :|