On 7/18/07, Havoc Pennington <<a href="mailto:hp@redhat.com" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">hp@redhat.com </a>> wrote:<div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> The hard thing is that the patch to call the callback at the end has to change every function in the call stack, more or less. Another headache here is that the connection lock has to be held until we stop using the connection->timeouts object, so maybe dbus_timeout_list_add_timeout is changed to return the app callback as an invokable closure or something of that nature, and then the closure is passed up the call stack until we can unlock and invoke it. The closure would store timeout_list->add_timeout_function and timeout_list->timeout_data, but could not have a pointer to the timeout_list itself since that is protected by the connection lock. </blockquote><div> Would this actually work?   I think this has other issues.   Such as in the Add_timeout case the user add_timeout function can fail and then we would need to regain the lock and remove the timeout from the list. If this was really how we wanted to do it I would see this as the closure and protected_protected_change_timeout would look like this. <pre>typedef struct DBusTimeoutClosure { DBusTimeoutAppCallback  app_callback; DBusTimeout* timeout; void* app_callback_data; DBusTimeoutFailureCallback failure_callback void* failure_callback_data }; static dbus_bool_t protected_change_timeout (DBusConnection *connection, DBusTimeout *timeout, DBusTimeoutAddFunction add_function, DBusTimeoutRemoveFunction remove_function, DBusTimeoutToggleFunction toggle_function, dbus_bool_t enabled) { DBusTimeoutList *timeouts; dbus_bool_t retval; DBusTimeoutClosure * closure; dbus_bool_t app_retval; HAVE_LOCK_CHECK (connection); if (add_function) retval = (* add_function) (connection->timeouts, timeout, &closure); else if (remove_function) { retval = TRUE; (* remove_function) (connection->timeouts, timeout, &closure); } else { retval = TRUE; (* toggle_function) (connection->timeouts, timeout, enabled, &closure); } _dbus_connection_ref_unlocked (connection); CONNECTION_UNLOCK (connection); if(closure) app_retval = closure->app_callback(timeout, app_callback_data); CONNECTION_LOCK (connection); _dbus_connection_unref_unlocked (connection); if(closure) if(!app_retval) { retval = closure->failure_callback(connection->timeouts, timeout, failure_callback_data); } return retval; }</pre></div> <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> A full fix for the reentrancy would probably involve adding a separate lock on the TimeoutsList or some nightmare like that. </blockquote><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">The simplest quick fix I can think of is to make timeout_list_add_timeout and friends return the closure thingy, then where the current code does timeouts=NULL and drops the lock, don't do the timeouts=NULL part since we won't need the timeout list, just the closure.</blockquote><div> I think what I describe above is close to this, but I'm not completely sure.  </div></div>The only other way I see (which might be cleaner) is to pass a function to lock and unlock the connection from protected_change_timeout to the DBusTimeout{Add,Remove,Toggle}Functions.  This would make them responsible for calling this unlock function before it call out to an application callback and to lock it right after. Keith Preston