Page MenuHomeFreeBSD

Use ratecheck(9) in in_pcbinslbgrouphash().
ClosedPublic

Authored by markj on Sep 6 2018, 5:43 PM.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bz added a subscriber: bz.

LGTM

This revision is now accepted and ready to land.Sep 6 2018, 6:29 PM
This revision was automatically updated to reflect the committed changes.

Hi. I know I'm late to the party, but I have three comments:
a) I could be wrong, but I don't think there is any guarantee this won't be called simultaneously for two different groups at the same time. (The groups could be in different VNETs, for example.) In that case, two different invocations could be working on the function's static variables at the same time. That may produce unexpected results. (Granted, it would take an unusual series of events. But, I think we've all seen highly unusual events occur.)
b) I don't think the const variable also needs to be static.
c) It seems like the rate limiter should really be per-group, so I would suggest adding the lastprint variable to the inpcblbgroup struct.

In D17065#364086, @jtl wrote:

Hi. I know I'm late to the party, but I have three comments:
a) I could be wrong, but I don't think there is any guarantee this won't be called simultaneously for two different groups at the same time. (The groups could be in different VNETs, for example.) In that case, two different invocations could be working on the function's static variables at the same time. That may produce unexpected results. (Granted, it would take an unusual series of events. But, I think we've all seen highly unusual events occur.)
b) I don't think the const variable also needs to be static.
c) It seems like the rate limiter should really be per-group, so I would suggest adding the lastprint variable to the inpcblbgroup struct.

Hmm, right, thanks. I'll fix this up and post a separate review.