[Telepathy] Review of Will's telepathy-glib requestotron branch

[Will Thompson]

On 17/09/08 22:49, Simon McVittie wrote:
> Ideal. I've re-reviewed and it all looks good (so just merge
> requestotron-trivia into requestotron), except:
>   
Merged, having fixed the “except:”s.
>>> c_r_r_validate_handle: tp_handle_ensure (usually? always?) raises
>>> NotAvailable on error, but CreateChannel is specified to raise
>>> InvalidHandle (slightly bending the usual semantics) if the TargetID is
>>> inappropriate - either propose a small spec patch, or transform the error
>>> (you can just alter error->code in-place, I think)
>>>   
>>>       
>> Fixed in 74c605f, altering error->code in place in the event of either
>> TargetID or TargetHandle being invalid.
>>     
>
> Can tp_handle_is_valid really ever raise something other than
> InvalidHandle? No harm in what you did, though.
>   
Oh, I missed that in the documentation of tp_handle_is_valid.
> However... tp_handle_ensure could theoretically raise any GError of any
> domain, so you should probably hack ->domain too.
>   
Fixed in b88305b.
>>> Any thoughts on a better (than POINTER) GType for a
>>> GHashTable<TpExportableChannel, GSList<request token> > as per the FIXME
>>> I put in exportable-channel.c? We can't dup request tokens, though (the
>>> TpChannelManager semantics are that the manager MUST forget about all requests
>>> that it has mentioned in a signal) so trying to invent a boxed type may just
>>> be an exercise in futility
>>>   
>>>       
>> Presumably we could use (something equivalent to, if dbus_g_type_map is
>> not meant to be use to construct random map gtypes):
>>
>>     dbus_g_type_get_map ("GHashTable", TP_TYPE_EXPORTABLE_CHANNEL,
>>     G_TYPE_POINTER);
>>
>> but it's not obvious what that would buy us. I don't see a GType for
>> GSLists, and again I don't really know what creating one would improve…
>>     
>
> No, I think you're right. Let's keep POINTER - please drop the FIXME
> comment.
>   
Fixed in bdc57dc.

A possible buglet occurred to me last night:
find_matching_channel_requests is used to figure out which requests are
satisfied or failed by TpChannelFactoryIface::new-channel and
::channel-error, respectively. But, in a pathological connection manager
where channels with the same ChannelType, TargetHandleType and
TargetHandle are sometimes managed by a TpChannelManager and other times
by a TpChannelFactoryIface, if the Factory signals something with that
CT, THT and TH first then the ChannelRequest known to the Manager will
be returned by find_matching_channel_requests. It will then be
prematurely answered and freed; when the Manager signals that token,
tp-glib will crash.

Ways to avoid this:

   1. find_matching_channel_requests could check that method ==
      METHOD_REQUEST_CHANNEL. This won't help the even more pathological
      case where a channel manager's _request_channel implementation is
      non-deterministic.
   2. add gboolean handled_by_channel_factory to ChannelRequest; then
      find_matching_channel_requests would only return requests with
      this set to TRUE.
   3. do nothing, let everything crash if your connection manager is insane.

I think I prefer 2., but am a bit reluctant to clutter the code with
defensive measures against insanity. Opinions?

-- 
Will


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: OpenPGP digital signature
Url : http://lists.freedesktop.org/archives/telepathy/attachments/20080918/044b37cf/attachment.pgp