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: [email protected]) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
