[PATCH v3] babel: Rework seqno request handling

[Toke Høiland-Jørgensen]

Ondrej Zajicek <santiago at crfreenet.org> writes:

> 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)) ...

This was actually on purpose: we want to keep the request object around
for longer, so we can use it to trigger an update when it's satisfied.
So I figured it would be simpler to just keep the expiry logic without
actually transmitting anything. There's a check in
babel_add_seqno_request() which uses sr->count time-limit the duplicate
check to one update interval (which is also shorter for forwarded
requests).

I'll update the comment to explain this.

>  - There is no need to check hop_count in babel_handle_seqno_request(),
>  that is already done in babel_read_seqno_request().

Lol. I went looking for that and somehow missed it; will remove.

>  - 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.

Right, can do; I was on the fence about whether I should move it back or
not, but I agree it makes more sense that way.

-Toke