[PATCH] Privatize the DBusPendingCall struct

[Havoc Pennington]

Hi,

Nice work, some minor comments:

John (J5) Palmieri wrote:
> +/** @} End of DBusPendingCallInternals */
> +
>  /**
>   * @defgroup DBusPendingCallInternals DBusPendingCall implementation details
>   * @ingroup DBusInternals

You can probably merge these two group sections (just move this defgroup 
up to replace the addtogroup)

>  /**
> + * Completes a pending call with the given message,
> + * or if the message is #NULL, by timing out the pending call.
> + * 
> + * @param pending the pending call
> + * @param message the message to complete the call with, or #NULL
> + *  to time out the call
> + */
> +void
> +_dbus_pending_call_complete (DBusPendingCall *pending,
> +                             DBusMessage     *message)
> +{

It might be good to rename this to set_reply() instead of complete() - 
complete() implies to me that it would also call the notifier.

> +/**
>   * Calls notifier function for the pending call
>   * and sets the call to completed.
>   *
> @@ -124,6 +189,18 @@

Not sure what function the above documents but it sounds more like 
complete() - I probably called it something weird

> +void
> +_dbus_pending_call_queue_synthesized_message_link_on_connection (DBusPendingCall *pending, 
> +                                                                 DBusConnection *connection)
> +{
> +  if (pending->timeout_link)
> +    {
> +      _dbus_connection_queue_synthesized_message_link (connection,
> +						       pending->timeout_link);
> +      pending->timeout_link = NULL;
> +    }
> +}

I'd call this maybe _dbus_pending_call_queue_timeout_error()

> +/**
> + * Sets the connection associated with the pending call
> + *
> + * @param pending the pending_call
> + * @param connection the connection which is associated with the pending call
> + */
> +void
> +dbus_pending_call_set_connection (DBusPendingCall *pending,
> +                                  DBusConnection *connection)
> +{
> +  _dbus_return_if_fail (pending != NULL);  
> +
> +  pending->connection = connection;
> +}

For this patch I'd leave this here and just make sure it's 
private/internal - it's good how you kept this patch as just a 
mechanical transformation of the previous code, rather than mixing 
moving stuff around and changing said stuff.

I agree with you though that this function should go away, it does not 
make any sense to change the connection to anything other than the 
original connection.

The trick if PendingCall holds a ref to the connection will be to avoid 
the refcount cycle. I think it's pretty simple; all the pending calls 
have to get completed on disconnect (via timeout or reply), and be sure 
pending calls can't be created while disconnected. IOW the invariant is 
that the number of pending calls on the connection must be 0 after the 
connection is disconnected.

> +
> +/**
> + * Sets the reply message associated with the pending call
> + *
> + * @param pending the pending_call
> + * @param reply the reply message associated with the pending call
> + */
> +dbus_bool_t
> +_dbus_pending_call_set_reply (DBusPendingCall *pending,
> +                              DBusMessage *reply)
> +{
> +  DBusList *reply_link;
> +  reply_link = _dbus_list_alloc_link (reply);
> +  if (reply_link == NULL)
> +    return FALSE;
> +
> +  pending->timeout_link = reply_link;
> +  
> +  return TRUE;
> +}

I would call this set_timeout_error or something like that; set_reply() 
I'd expect to be used for setting the actual reply.

Havoc