[PATCH] babel: Don't try to remove multicast seqno request objects from neighbour list

[Ondrej Zajicek]

On Thu, May 12, 2022 at 02:12:13PM +0200, Toke Høiland-Jørgensen wrote:
> The Babel seqno request code keeps track of which seqno requests are
> outstanding for a neighbour by putting them onto a per-neighbour list. When
> reusing a seqno request, it will try to remove this node, but if the seqno
> request in question was a multicast request with no neighbour attached this
> will result in a crash because it tries to remove a list node that wasn't
> added to any list.
> 
> Fix this by making the list remove conditional. Also add a check so that
> seqno requests are only reused if the neighbour also matches, allowing
> multiple outstanding requests for the same router ID.

Hi

I wanted to merge the patch but i found some discrepancies in seqno
request handling that are confusing:

1) RFC 6126/8966 describes that entries in the table of pending requests
have "the neighbour, if any, on behalf of which we are forwarding this
request". In contrast, the 'nbr' field in struct babel_seqno_request
describes the neighbor to whom we send the request.

2) It seems that we do not keep the 'neighbor on behalf we are forwarding',
not sure if that is important or not.

3) The stored neighbor is used just for resending requests when they timeout.

4) Your patch changes it to allow multiple pending requests to different
neighbors, but why? Seems to me that we should send requests (for fixed
entry and router id) to only one neighbor (the current best one).

5) When an iface is removed, neighbor is removed, which also leads to
removal of all associated seqno requests. Is this correct approach?
Shouldn't we instead resend pending requests to the new best neighbor
for that entry+router_id?


Seems to me that we could just eliminate the nbr pointer from the struct
babel_seqno_request, remove associated bookkeeping (per-neighbor list and
code handling it). When a seqno request timeouts and must be resent, we
can just find the appropriate neighbor from route entries. This
simplifies the code and also handles 5). Is this reasonable or are there
any issues with this approach?

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at 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."