I'll respond now while I'm looking at this, as Dave may not be able to get to this until later today I was talking with Dave about this yesterday, and if I'm not mistaken, this idea comes from some of the implementations for either DevKit, DBus, or libHAL (I can't remember which one he was looking at at that particular moment) Essentially, if I understand the model correctly, they have 2 fd sets - one for reading, one for writing. I could see an additional use where you have multiple callbacks - one for data, and one for error handling. These 2 calls to AddHandle would take the sale fd, but would have different event mask sets, for the different callbacks. It does make the model with the timeouts more symmetric. -----Original Message----- From: libvir-list-bounces(a)redhat.com on behalf of Daniel P. Berrange Sent: Tue 11/4/2008 5:10 AM To: Dave Lively Cc: libvir-list Subject: Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent On Mon, Nov 03, 2008 at 10:51:36AM -0500, David Lively wrote: IMHO it doesn't make sense to register 1 file descriptor multiple times - once the I/O has been processed by one handler, there's nothing more another handler can do. I think there are a few options if the FD is already registered when calling 'add' - Return an error code - Ignore the new request - Remove the existing callback - Say behaviour is undefined Trying to define any more complex semantics than this will limit our ability to integrate with existing event loop implementations. We currently defacto do option 3, which is fine provided no libvirt code ever registered the same FD more than once - which we don't. I think I'm inclined to stick with saying behaviour is undefined, and if a libvirt driver needs to somehow process the FD events in multiple places it can deal with that at a higher level - eg it can register one callback to get the notifications & perform the I/O, and then fire off some notifications to the bits of the driver than ned to know when the I/O is complete. 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(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2008-11-05 at 11:51 +0000, Daniel P. Berrange wrote: 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. Dave

Ugh. I see what you mean. It seems more complicated than that, though. For one thing, the AvahiWatch/AvahiPoll stuff is sufficiently abstract that it doesn't mention DBusConnection (or any other DBus type, AFAIK), so there's no indication that *any* DBusConnection is being used (though I trust that there is). Turning on AVAHI_DEBUG, I see both my (node_device_hal.c) callbacks and the avahi callbacks being called (but at different times, and with different fds and event sets). Makes me wonder whether the avahi library is using dbus_bus_get_private() to get its own DBusConnection that it doesn't have to worry about sharing with others. I tried changing the node_device_hal code to use a private dbus connection, and see exactly the same behavior (i.e., dbus immediately registers the same fd twice, with different event sets and enable state). Regardless, I like the idea of simply using this private connection (as you suggested but rejected), just for the sake of simplicity. (Also, what if Avahi's been configured out?) But ... back to the original problem. Immediately after calling dbus_set_watch_functions (and *before* initializing the Avahi stuff), I see two callbacks to watch_add with the same fd. Looking at the glib watch functions, I see watches on GIOChannels are added with g_io_add_watch(), which indeed doesn't specify what happens if the same GIOChannel is added twice. But GIOChannels aren't fds. Presumably, one could create two different GIOChannels with the same fd (via g_io_channel_unix_new) and then register watches on each of them. So I remain convinced that we need to support the registration of the same fd multiple times ... Dave [Caveat: Okay, so I've never actually *used* glib ... I'm going from the docs here ...] On Wed, 2008-11-05 at 16:49 +0000, Daniel P. Berrange wrote:

I'll answer for Dave, while I'm looking at this. As far as I know, Dave is of the opinion that we are just "getting lucky" using the APIs as we are, and remains convinced that his suggested change is necessary here. He (and I) remain worried that release of the EventImpl API without this API change could get us into trouble in the future, as we would have to support the released API that has different semantics than DBus, which we were supposed to be modeled closely to. You had sounded convinced it was not necessary the last we heard though...and ultimatley we don't have checkin permissions...so we'll go with whatever you guys decide. I'll let Dave follow up with any additional examples of where this would get us in trouble... -----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Fri 11/14/2008 6:27 AM To: Dave Lively Cc: Ben Guthro; libvir-list Subject: Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent On Wed, Nov 05, 2008 at 05:32:36PM -0500, David Lively wrote: Did you ever get to the bottom of this problem. If not, then I think the only safe thing todo for this release is to change the signature of AddHandle as you suggest, so we explicitly track FDs using an integer ID that's independant of the FD number. 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 :|

On Fri, 2008-11-14 at 14:11 +0000, Daniel P. Berrange wrote: Yep. Thank you! If you'd rather, I'd be happy to make those changes (today) and submit a patch. (But I haven't implemented them yet, other than changing the decls and documentation.) Dave

On Fri, Nov 14, 2008 at 10:46:22AM -0500, David Lively wrote: Don't worry, I've already got it in progress myself.... 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 :|

I'd like to get the Xen driver domain events in that release as well. I'm working through some bugs with these, but I think it should be ready for review sometime this week, or beginning of next.