On Sat, Dec 17, 2022 at 09:45:59PM +0100, Toke Høiland-Jørgensen via Bird-users wrote:
The seqno request retransmission handling was tracking the destination that a forwarded request was being sent to and always retransmitting to that same destination. This is unnecessary because we only need to retransmit requests we originate ourselves, not those we forward on behalf of others; in fact retransmitting on behalf of others can lead to exponential multiplication of requests, which would be bad.
Hi Thanks for investigating the issue and the patch. It took me some time to process it, so answering now. I have some notes / suggestions: - In babel_expire_requests() the check for sr->forwarded is inside sr->count check, so BABEL_SEQNO_REQUEST_RETRY is applied for forwarded info, which does not make sense, perhaps it should be like: if (!sr->forwarded && (sr->count < BABEL_SEQNO_REQUEST_RETRY)) ... - There is no need to check hop_count in babel_handle_seqno_request(), that is already done in babel_read_seqno_request(). - Seems to me that as the focus of the patch changed completely from v1 to v3, some changes thate were initially introduced now makes less sense. Namely i would keep meaning of the last argument of babel_add_seqno_request() to be the neighbor we forward the request to, and keep seqno request routing (i.e. babel_find_seqno_request_tgt()) outside. Either keep old code in babel_handle_seqno_request(), or call babel_find_seqno_request_tgt() from that. That makes more sense to me as it is part of section 3.8.1.2 implemented there. -- 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."