[Telepathy] OLPC branch review

[Robert McQueen]

I reviewed the OLPC branch yesterday:
 http://projects.collabora.co.uk/~monkey/telepathy-gabble-olpc

I gave this list to Guillaume already so he's already fixed some stuff here:
http://projects.collabora.co.uk/~monkey/telepathy-gabble-olpc-rob-review/

But I thought I'd let people know what was going on by posting it here
too. :)

Regards,
Rob

debug.c: I don't like the capital letter debug flag

conn-aliasing.c; setaliases_foreach: do a send_with_reply even if the
callback just prints a warning if there's an error

gabble-connection.c: XXX: this should be generated... generate it then?

connection_msg_callback: this is at least poorly named, or it should
actually live outside the connection in a conn-pep.c of some
description. it also has the side-effect of swallowing all messages
whose first <event> contains no XMLNS... use early return for null
event_ns and wrong prefix, which will avoid nested ifs too. also it
calls tp_handle_unref in the path where it's already known to be 0 (odd
spacing there too).

olpc.c/h: missing copyright headers. should this not be conn-olpc.c anyway?

global hash tables? nyet. store these per connection and free them when
we disconnect. secondly, the contacts_activities hash table is indexed
by handle but not within the context of a given connection, which is
totally wrong. as mentioned on channel, members of gabble connection, or
qdata would be better.

typedef'd structs should be CamelCase, and you can make an anonymous
typedef anyway (typedef struct { ... } ActivityInfo).

activity_info_new: use slice_new0 to avoid unexplained madness in
future. odd spacing of * in prototype. why dup the string version of the
room if you have the handle and the repo? you can always look it up. the
activities_info hash could then be indexed by handle, saving memory and
strcmps.

function prototypes in this file are inconsistent, see
update_activities_properties vs activity_info_new vs
activity_info_set_properties (which is correct gabble style)

inspect_contact should call _is_valid which will can return an error for
you too; inspecting an invalid handle is a shooting offence

check_publish_reply_msg: we have NODE_DEBUG to print a node, but its
rarely worth using at all, because you can always run with LM_DEBUG=net
to find out the reason for errors. maybe use some xmpp_error stuff here
to print just a reason string. ditto check_query_reply_msg.

extract properties: make an early return if props_node is null. in the
bytes bit it leaks the decoded GString, then leaks the GArray too. use
_take_boxed. is it really sane to assume a NULL type is bytes, and is
base64_decode safe against NULL value? maybe just skip anything where
type and value are unset.

---

add_activity_info should take a handle, and the person who calls it
responsible for validating the room in some way. passing potentially
invalid info around in internal API is easy to mess up.

_set_activites: passes untrusted handle into _inspect without
verification (check for more of these errors?). inspecting a handle
should never return null. consider an early return/continue too.

_set _current_activity: again, fails to validate given room handle.
error should say "can't set an activity as current if you're not
announcing it" or something..

inspect_room: assumes handle validity

_set_properties: "can't set properties on an activity if you're not
announcing it"

check_prop_in_old_properties: (maybe?) use tp_strdiff, string GValues
can contain NULL

... this file is huge, could we actually split it into buddy info and
activity properties? would help ensure that they're not too tightly
coupled when we want to consider replacing activity properties with
something else.

update_activities_properties: use a for loop, then strcmp should cause
an early continue; not another load of nesting. it currently uses
continue in a while loop without following the next pointer, which will
just result in infinite looping. :P

gabble_pubsub_event_handler maybe doesn't need a return type, or if it
does, it should just return whether or not the event namespace was
dispatched to a handler, which is a useful thing to know, not whether
the handler was "successful" (you don't know then whether the handler
has dispatched a meaningful error or if we should reply somehow)