Phantom "Out of Memory" error

[keith preston]

On 7/18/07, Havoc Pennington <hp at redhat.com> wrote:

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


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.

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;


}


A full fix for the reentrancy would probably involve adding a separate
> lock on the TimeoutsList or some nightmare like that.
>
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.


I think what I describe above is close to this, but I'm not completely sure.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.freedesktop.org/archives/dbus/attachments/20070718/f11c3841/attachment.htm