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

Will Thompson will.thompson at collabora.co.uk
Wed Sep 17 10:35:16 PDT 2008


gOn 17/09/08 05:51, Simon McVittie wrote:
> Reviewed asynchronously due to Linux Plumbers Conference travel - greetings
> from the skies over, er, Minnesota, I think? Obviously, the actual
> transmission of this email will be (or was, from your perspective) somewhat
> later...
>
> I reviewed up to commit afa0599419bfd12b2663339673d469da37052623 (I'd have
> looked for a newer version, but Three connectivity in Heathrow airport
> seems to fail).
>   
Really? That's an unrelated commit made by you a week ago, just before
the merge of the self-handle-changed branch, according to my git log…
> Things marked [trivia] or [later] are not necessarily merge or release
> blockers, given the importance of getting the requestotron stuff moving, but
> should be done sometime (for [trivia] just do them in a trivia branch,
> or convince me I'm wrong; for [later] file a bug). That covers most of it,
> in fact.
>   
I have made a requestotron-trivia branch, based on the requestotron
branch, in which I have fixed the [trivia]-annotated items, to make
reviewing the fixes to less trivial complaints more straightforward.
> signals-marshal.list
> ====================
>
> To simplify merges, in files like this one where order is totally
> irrelevant, please use asciibetical order (vim: :1,$!LC_ALL=C sort) -
> any consistent order is fine, I just picked this one arbitrarily
>   
Fixed in commit 70f70f8.
> gtypes docs
> ===========
>
> Please merge the Requests Private subsection containing the underlying
> functions (tp_type_dbus_array_a_7bsv_7das and friends) into the existing
> subsection containing the other similar functions. The public #defines
> like TP_HASH_TYPE_CHANNEL_CLASS are conceptually part of the Requests
> interface, so giving them their own subsection is good, but the underlying
> functions are not specific to any interface, and so should all go together
> in LC_COLLATE=C order (also, this simplifies merges).
>   
Moved and sorted in commit 72670a9.
> base-connection.c
> =================
>
> [trivia] I'd be inclined to fold the copyright years into a range
> (Copyright (C) 2005-2008...), it's starting to get quite stupid
> otherwise :-)
>   
Fixed in 1e0ced22b893be31686306b3635fcd7d091c5a9f, which also bumps the
copyright on channel-factory-iface.h which I noticed was necessary while
editing it.
> [trivia] While you're touching the <telepathy-glib/*> headers at the
> top anyway, please sort them ASCIIbetically.
>   
Fixed in c15ca2f
> [trivia] The addition of requests_iface_init() means that
> service_iface_init should probably be renamed to conn_iface_init (like
> it should have been all along, but I was young and naive).
>   
The errors of your youth have been wiped away in 72fd131.
> [trivia] To make the intention completely clear, get_channel_details()
> should probably use TP_HASH_TYPE_QUALIFIED_PROPERTY_VALUE_MAP rather
> than TP_HASH_TYPE_STRING_VARIANT_MAP, now we have it.
>   
Fixed in fbaa44f.
> [trivia] get_channel_details() should allocate structure to be of length
> 2, since it always gets 2 members :-)
>   
Fixed in 451be6f.
> satisfy_requests() is a bit confusingly named - it's only called for
> TpChannelFactory channel creations. Rename it to factory_satisfy_requests
> or something? (satisfy_request (singular), on the other hand, is fine!)
>
> Similarly channel_closed_cb, connection_new_channel_cb,
> connection_channel_error_cb should probably be factory_channel_closed_cb,
> factory_new_channel_cb, factory_channel_error_cb
>   
Both fixed in d8c525d.
> [later?] tp_base_connection_close_all_channels: please discuss with
> Rob whether we want to give channel managers a close_all callback, or
> add a signal disconnect-imminent(guint reason), to give them a chance
> to emit Closed (or perhaps a Group change, if that's more meaningful)
> just before status-changed goes out. If we do want such a callback,
> we can add it as part of the EnsureChannel round of changes, though,
> so this isn't a release blocker IMO. Also, TpChannel will do the
> right thing even if the Channel Closed signal never comes (it'll become
> invalidated with an error in domain TP_ERRORS_DISCONNECTED whose code is
> a TpConnectionStatusChangedReason, I believe - and that's at least as
> informative as anything the Channel could come up with on its own!) so
> perhaps we don't need that.
>   
https://bugs.freedesktop.org/show_bug.cgi?id=17631 is assigned to me.
> RETURN_INVALID_ARGUMENT should be of the form do {...} while (0), or
> better G_STMT_START {...} G_STMT_END (or whatever they're really called
> - I can never remember), so that compilers in ultra-pedantic mode won't
> complain about "RETURN_INVALID_ARGUMENT(...);", i.e. "{...};", which is
> a block "{...}" followed by the useless null statement ";"
>   
Fixed in 0d1a911.
> conn_requests_check_basic_properties should require that TargetHandle
> is nonzero if present, so the complaint about TargetHandle should say
> "in range 1 to 2**32-1"
>   
Already fixed in 7ddd4f68c9e94bf8c1e07d1e637cbabe54aa7a43.
> conn_requests_requestotron_validate_handle: I think it's worth having
> separate error returns (with different messages) for "when requesting
> a TargetHandleType, you must also specify either a TargetHandle or a
> TargetID" and "TargetHandle and TargetID must not both be given"
>   
Fixed in 7dd6835.
> 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.
> conn_requests_offer_request: note for the future that the behaviour of
> having suppress_handler = FALSE for Ensure requests is debatable - either
> think about this some more, or put a FIXME comment in your Ensure branch
>   
I think that the issue that you're referring to here is that in the case
where Ensure returns Yours=True to someone, suppress_handler should be
True. I've added a FIXME in my local Ensure branch.
> [trivia] in conn_requests_offer_request I'd exchange "request->context =
> NULL" with the preceding blank line, to make it more obvious that the
> async return is exactly the reason that you may (and must) do this
>   
Fixed in 40c2df1
> [trivia] end of tp_base_connection_channel_manager_iter_next: the style that
> daf and I have been encouraging would put a blank line between the if-block
> and the line "iter->index++"
>   
Fixed in ceaf77a.
> [trivia] channel-manager.h, exportable-channel.h: no gtkdoc in headers
> in new code please (to reduce recompilations and -dev package size) -
> move them to the .c
>   
Fixed in 67cc460 and 9f49ba0 respectively.
> [trivia/later] base-connection.h: moving gtkdoc to the .c (to reduce
> recompilations and -dev package size) is encouraged, feel free to do
> this at any convenient time
>   
Fixed in 257e928.
> TpBaseConnectionClass struct docs: Maybe it's too early to document
> create_channel_factories as deprecated - "In new code, setting this to
> NULL is recommended" is a weaker form? We certainly don't want anyone
> trying to make it G_GNUC_DEPRECATED!
>
> Similarly, it may be too early to deprecate TpChannelFactory.
>   
I think you're right on both counts; fixed in cf46296.
> 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…
> [later] please enable Requests in the echo CM (the only example CM with
> proper channels, currently) so there's a minimal example
>   
https://bugs.freedesktop.org/show_bug.cgi?id=17631
> TpChannelManagerRequestFunc: if TargetHandleType is None, TargetHandle
> is omittedG_TYPE_BOXED
>   
Already fixed in f6572ec05eb782ea5d964549bddc424ce638c7b0
> TpChannelManagerIface doc: "thirteen GCallback fields" is in fact a lie,
> there are "only" 11 - but you might as well just say "several" tbh
>   
Fixed in e0244ad58f3ebf739fa0282c3ad8b2516f2d28f9. Whoops.
> TpChannelManagerIface _near_future: perhaps replace
> with GCallback _reserved_for_ensure_channel, GCallback
> _reserved_for_foreach_contact_channel_class, and whatever the other caps
> function is?
>   
Fixed in 3b96eb1ec1f461077fed413522a8047277c98f83
> tp_dbus_properties_mixin_make_properties_hash: When termination
> is reached (i.e. interface is NULL), property points sizeof(char*)
> bytes past interface, to some misc bit of stack (in practice, most
> likely to be platform calling convention internals like the saved IP,
> which makes my security brain cell itch, even though it's clearly not a
> problem when this function is used correctly). I suggest you move the
> update of property to the loop body. Even if you overrule me on that,
> in the loop body you need to assert (or at least return_val_if_fail)
> that property != NULL, to avoid the potential for an an off-by-one
> (accidentally concatenating two of the strings by omitting the comma,
> perhaps) which would cause us to miss the NULL terminator entirely.
>   
Fixed in c371fee18e63bc19bb8b33f4261d54010a38355b
> General
> =======
>
> After merging this lot, please write an interim NEWS entry noting that
> tp-glib claims to implement spec 0.7.11, with initial Requests support
> (CreateChannel but not EnsureChannel) - it's probably worth mentioning
> those functions by name in NEWS. At that point it's good for a release
> whenever, I think, but please let me review the release preparation - I
> did all the previous releases, and the process is probably rather subtle
> (and/or insane).
>   
I've added a couple of NEWS entries in eb424a3 at the end of the
requestotron-trivia branch, but haven't run around doing
s/UNRELEASED/15/ etc., and need to leave the internet now. I'll do the
rest of the release preparation tomorrow if you haven't done it by then. :-)

Happy plumbing,

-- 
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/20080917/b721c116/attachment-0001.pgp 


More information about the Telepathy mailing list