Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Wed, Nov 05, 2008 at 05:32:36PM -0500, David Lively wrote: 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 ... 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
RE: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
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:[EMAIL PROTECTED] 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: 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 ... 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Fri, Nov 14, 2008 at 07:36:27AM -0500, Ben Guthro 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. Basically, there is no downside to implementing your suggestion of allowing the same FD to be registered, and a clear potential downside to our current impl. So I'll re-write the Add/RemoveHandle API as you suggested to eliminate the risk 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] [RFC] making (newly public) EventImpl interface moreconsistent
On Fri, Nov 14, 2008 at 10:46:22AM -0500, David Lively wrote: On Fri, 2008-11-14 at 14:11 +, Daniel P. Berrange wrote: On Fri, Nov 14, 2008 at 07:36:27AM -0500, Ben Guthro 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. Yep. Basically, there is no downside to implementing your suggestion of allowing the same FD to be registered, and a clear potential downside to our current impl. So I'll re-write the Add/RemoveHandle API as you suggested to eliminate the risk 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.) 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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Tue, Nov 04, 2008 at 07:04:54AM -0500, Ben Guthro wrote: 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. 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. 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. QT allows this model for its QSocketNotifier class, indeed it actually requires it, but GLib leaves this unspecified, and I've not been able to determine from the code whether its possible to register the same FD twice with GLib. 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] [RFC] making (newly public) EventImpl interface moreconsistent
On Wed, 2008-11-05 at 11:51 +, 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.
Dave
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
On Wed, Nov 05, 2008 at 11:26:07AM -0500, David Lively wrote:
On Wed, 2008-11-05 at 11:51 +, 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 :|
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
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 +, Daniel P. Berrange wrote: On Wed, Nov 05, 2008 at 11:26:07AM -0500, David Lively wrote: On Wed, 2008-11-05 at 11:51 +, Daniel P. Berrange wrote: 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 -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
RE: [libvirt] [RFC] making (newly public) EventImpl interface moreconsistent
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: [EMAIL PROTECTED] 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: Hi Folks - Since virEvent???RegisterImpl is now public (in libvirt.h), a nagging concern of mine has become more urgent. Essentially this callback gives clients a way of registering their own handle (fd) watcher and timer functionality for use by libvirt. What bugs me is the inconsistency between the handle-watcher and timer interfaces: the timer add function returns a timer id, which is then used to identify the timer to the update and remove functions. But the handle-watcher add / update / remove functions identify the watcher by the handle (fd). The semantics of registering the same handle twice aren't specified (what happens when we pass that same fd in a subsequent update or remove?). Even worse, this doesn't allow one to manage multiple watches on the same handle reasonably. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
