In D59305#1360809, @cc wrote:

In D59305#1360805, @tuexen wrote:

What about the RACK and BBR stack?

They don't seem to be impacted or relevant.

static void rack_post_recovery(struct tcpcb *tp, uint32_t th_ack);

static void bbr_post_recovery(struct tcpcb *tp);

What about the RACK and BBR stack?

Committed in c6ae11c1b0d9.

In D58594#1344809, @rscheff wrote:

Looks like you do modify the BBR/RACK stack too, unlike the high level description. In both places the same checks used to be done in a mirror image - and this again mirrors each other.

As suggested by glebius.

To whom it may concern: the same problem seems to exist in the Linux source code: cm.c.

Fix white space issue.

More cleaner version.

In D57817#1327987, @glebius wrote:

Yes, patch as is doesn't improve anything. The comment above the function clearly marks the intent: the function checks if packet should be dropped and if so provides error code. It doesn't provide error code in case packet shouldn't be dropped.

P.S. For the sake of human comprehension I would make the function bool and rename the ret_val to error.

I don't see what problem this patch should solve nor do I think at compiles at all, since the variable at is not declared in sack_filter_blks(). Please describe which bug you want to fix. Also, when providing a patch, provide context. If you generate the patch file using git diff, please use git diff -U999999 instead.

ret_val is only considered by the caller if ctf_drop_checks() returns 1. So there is no problem in leaving it unassigned when returning 0. So the proposed fix would not change the behavior. So I would tend to not add the proposed change, but let us get glebius into the loop. Maybe improving the comment is the best way forward.