Juliusz Chroboczek <j...@irif.fr> writes: >> The problematic bit is, I think, 's' in babel_handle_update can be NULL >> because nothing ensures the babel_source for a particular neighbour >> actually exists here: > > s will be passed to babel_is_feasible, which returns true if s is null. > Later on, s is only used if feasible is false, in which case it cannot be > null. > > See RFC 8966 Section 3.5.1: > > a route advertisement carrying the quintuple (prefix, plen, router-id, > seqno, metric) is feasible if one of the following conditions holds: > > • ... > • no entry exists in the source table indexed by (prefix, plen, > router-id); > • ... > > I agree that the code is a little too subtle for comfort.
Pish posh, there's a totally-obvious comment saying /* for feasibility */ next to where 's' is assigned :P >> Perhaps find should just be replaced by babel_get_source here? > > babel_get_source sets the seqno to an arbitrary value (0), so it should > only be used if it is immediately followed by an assignment to s->seqno. > A better API would be to pass the seqno to babel_get_source. Hmm, yeah, possibly from a readability PoV, but, well, there's only the single caller, so it becomes a bit redundant since we have to do the check anyway afterwards. And I don't think switching babel_handle_update() to use babel_get_source() is a good idea either; we'd end up creating new source objects and leave them to be garbage collected just to improve readability a bit; just add a comment explaining why the deref is safe? :) > (I haven't looked at it in detail, but the code in babel_send_update_ looks > incorrect to me, by the way: it's comparing seqnos as integers, where it > should be doing comparisons modulo 2¹⁶, as defined in Section 3.2.1. > Toke?) You're right about this, though; same in babel_is_feasible(). Nice catch! Will send a patch... -Toke