[PATCH] Privatize the DBusPendingCall struct

[John (J5) Palmieri]

Take 2. I'm going to go ahead and look into keeping a ref for the
connection.  If the patch is approved I will commit it.

On Mon, 2006-07-10 at 21:37 -0400, Havoc Pennington wrote:
> 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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus-privatize-pending-call2.patch
Type: text/x-patch
Size: 21544 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20060711/bb2c4e42/dbus-privatize-pending-call2-0001.bin