On Sat, Apr 30, 2016 at 01:20:14AM +0200, Toke Høiland-Jørgensen wrote:
Hi
I would like to thank Toke Høiland-Jørgensen for the Babel protocol implementation, which was finally merged despite my tardy code reviews.
Yay, awesome! You're very welcome, and congratulations on the release! :)
Hi Some comments to my changes. I originally thought that i would just do some minor formatting changes, but i end with some heavy code changes and plenty of bugfixes. Hopefully i did not make much more errors. Some issues i found and fixed (and noted down): - modified ge_mod64k() still does not work: static inline u16 ge_mod64k(u16 a, u16 b) { return (a == b || b - a > 0x8000); } Due to C integer promotion rules, b - a is computed in (signed) integers and not in u16s, so if b = 1 and a = 2, you get -1, so > is false. The solution is to use uint as the type for a, b, so it is computed in unsigned integers and result can be safely typecasted to u16: static inline int ge_mod64k(uint a, uint b) { return (u16)(a - b) < 0x8000; } - in some cases (e.g. babel_handle_route_request()), check for plen == 0 or check if prefix is IPA_NONE is used where check for AE 0 should be used instead. Note that these are two different cases - there is the default route ::/0 (with AE 2), which is a regular route, and there is 'wildcard' AE 0, which represent all routes. - p->entry_slab allocated but not used - struct babel_tlv_header - should also be packed (AFAIK there are some archs that has structures padded to alignment of at least int). - babel_get_attr() - router-id is printed from a->u.data, but it is stored in a->u.ptr->data instead (one more indirection) - babel_process_packet() - check for framing error accesses tlv->length without checking that it can be accessed (i.e., if there is only one remaining byte, the tlv->length is out of bounds). - babel_process_packet() - Pad1 TLV is not correctly handled. - babel_process_packet() - memory leak in sl_alloc() / sl_free() for 'cur' variable - if the TLV parsing loop ends correctly, it is not freed. - babel_send_unicast() - if there is a packet waiting in the socket and another TLVs in the queue, the packet is overwritten and TX sequence for multicast packets in queue is interrupted. - log(L_ERR, ...) should be usually either log(L_REMOTE, ...), or in some cases TRACE(). - babel_read_router_id(), babel_read_next_hop() changed to PARSE_IGNORE as these functions just modify the state and do not generate parsed data. - babel_read_router_id() - endianity problem for router_id (the value is read directly without using get_u64()). - babel_read_ihu() - supposes that addr field is zeroed, but nobody zeroes it. - babel_read_route_request() - non-wildcard may have plen 0 (route ::/0) - babel_read_update() - may read after buffer if BABEL_AE_IP6_LL and omitted are set - babel_read_update() - does BABEL_AE_IP6_LL make sense? - babel_write_queue() - there is a queue argument, but the function processes ifa->tlv_queue anyways. So i guess unicast messages did not work. Also the queue argument uses call by value, should use call by reference. - babel_handle_update() - in 'if (msg->router_id == s->router_id) return;' should be r->router_id instead of s->router_id. (as the source and msg have always the same router-id value). Feel free to comment any of my changes. I found also some problems that were hard to fix and not severe, mainly related to route recalculation and triggered updates. These were left in the code. I will comment them later. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."