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) _______________________________________________ Telepathy mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/telepathy
