|
Description
Simon McVittie
2011-01-21 09:31:50 UTC
I have a branch for this, built on top of the one from Bug #33336; gitweb is in the URL field. Review would be appreciated. I think this is a job for D-Bus 1.5, really; I'd like to branch 1.4 soon so we can make this sort of change in master. (In reply to comment #0) > For watches, bus/activation.c has some extra madness in the callback, with a > big comment explaining why it shouldn't. Eventually, it just shouldn't This turned out to be easier than I thought, so I fixed it. Created attachment 42374 [details] [review] [1/11] DBusConnection: ref the connection in the timeout handler client_timeout_callback in bus/test.c refs the connection across the timeout invocation, which looks suspiciously like a workaround. If we make the timeout handler itself ref the connection, we won't need that, and can simplify timeout handling drastically. Bug: Created attachment 42375 [details] [review] [2/11] DBusLoop: remove a layer of pointless abstraction around timeouts Instead of supplying 8 tiny wrapper functions around dbus_timeout_handle, each with a user_data parameter that's a potentially unsafe borrowed pointer but isn't actually used, we can call dbus_timeout_handle directly and save a lot of trouble. One of the wrappers previously called dbus_timeout_handle repeatedly if it returned FALSE to indicate OOM, but that timeout's handler never actually returned FALSE, so there was no practical effect. The rest just ignore the return, which is documented as OK to do. Bug: Created attachment 42376 [details] [review] [3/11] DBusLoop: remove second layer of watch callbacks where possible Similar to the previous commit, almost every use of DBusWatch can just have the main loop call dbus_watch_handle. The one exception is the bus activation code; it's had a comment explaining why it's wrong since 2003. We should fix that one day, but for now, just migrate it to a new _dbus_loop_add_watch_full which preserves the second-layer callback. Bug: Created attachment 42377 [details] [review] [4/11] bus-activation: separate the "finished" callback from the watch callback This has been marked as broken since 2003... Bug: Created attachment 42378 [details] [review] Remove _dbus_loop_add_watch_full Bug: Created attachment 42379 [details] [review] [6/11] DBusLoop: factor out watch_flags_to_poll_events, watch_flags_from_poll_revents Bug: Created attachment 42380 [details] [review] [7/11] DBusLoop: keep separate lists of watches and timeouts Bug: Created attachment 42381 [details] [review] DBusLoop: move OOM watches to a secondary list instead of flagging them This will eventually let us maintain a DBusPollFD[] of just the active watches, between several iterations. The more immediate benefit is that WatchCallback can go away, because it only contains a refcount, a now-useless type, and a watch that already has its own refcount. Bug: Created attachment 42382 [details] [review] DBusLoop: inline add_callback, remove_callback into their callers The watch and timeout code paths will diverge completely when we change WatchCallback * to just be a DBusWatch *, removing the benefit of having common code. Bug: Created attachment 42383 [details] [review] Drop WatchCallback entirely, just use a list of DBusWatch in the main loop Bug: Created attachment 42384 [details] [review] [11/11] DBusLoop: fold Callback into TimeoutCallback Bug: (In reply to comment #12) > Bug: Sorry, please ignore these lines; git-bz ate my http://dep.debian.net/deps/dep3/ annotations. :-( (In reply to comment #9) > DBusLoop: move OOM watches to a secondary list instead of flagging them Unfortunately, this patch isn't right: if a watch is OOM on iteration n, and then it's removed by a different watch/timeout's callback during iteration n+1, it'll still be in the temporary list oom_last_time, so it won't be removed correctly. Created attachment 42450 [details] [review] [5/11] Remove _dbus_loop_add_watch_full Slightly adjusted to work better with the revised versions of patches 8-11. Created attachment 42451 [details] [review] [8/11 v2] DBusLoop: move OOM flag in watches inside the DBusWatch This will eventually let us maintain a DBusPollFD[] of just the active watches, between several iterations. The more immediate benefit is that WatchCallback can go away, because it only contains a refcount, a now-useless type, and a watch that already has its own refcount. This doesn't take any more memory for DBusWatch when not using DBusLoop (e.g. in client/service code or bindings), because we're just using more bits in an existing bitfield. (This fixes Comment #14.) Created attachment 42452 [details] [review] [9/11 v2] DBusLoop: inline add_callback, remove_callback into their callers Adjusted to apply after 8/11 v2. Created attachment 42453 [details] [review] [10/11 v2] Drop WatchCallback entirely, just use a list of DBusWatch in the main loop Adjusted to apply after 8/11 v2. Review of attachment 42374 [details] [review]: Looks good. Review of attachment 42375 [details] [review]: Looks good. Review of attachment 42376 [details] [review]: Looks good. Review of attachment 42377 [details] [review]: Looks good. I guess the next commit removes _dbus_loop_add_watch_full? Review of attachment 42377 [details] [review]: Looks good. I guess the next commit removes _dbus_loop_add_watch_full? Review of attachment 42450 [details] [review]: Looks good. Review of attachment 42379 [details] [review]: Looks good. Review of attachment 42379 [details] [review]: Looks good. Review of attachment 42380 [details] [review]: Looks good. Review of attachment 42451 [details] [review]: Looks ok. Review of attachment 42452 [details] [review]: Looks good. This is actually cleaner anyway. Review of attachment 42453 [details] [review]: Looks good. Review of attachment 42384 [details] [review]: Looks good Fixed in git for 1.5.6. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.