Hello Klemens,

</snip>

> `ptr_array' is allocated momentarily through mallocarray(9) and gets
> filled with the TAILQ entries, the sole user pfsync(4) can then access
> the list of rules by index to simply pick the n-th rule in a ruleset
> during state insertion.
> 
> I came here due to the zero size in its free(9) call, but the diff below
> removes the array completely and makes pfsync iterate over the TAILQ to
> get "index" of the matching rule in the ruleset.
> 
> I've been successfully running this on amd64 firewalls with pfsync(4)
> where I also added expiring `once' rules to further test with.
> 
> Feedback? OK?
> 

    my only concern is performance impact. your diff trades O(1) for O(n) just
    to handle expiration of PFRULE_ONCE correctly. However the extra costs here
    might be bit reduced by fact, we do the look up just in case receive brand
    new state from peer.

    looking at the problem from different angle:

        if there would be no PFRULE_ONCE rules, the bug won't exist

    I vaguely remember the question of removing 'PFRULE_ONCE' rule is still
    unanswered. It seems to me the PFRULE_ONCE is supposed to be used by
    tftp-proxy only. However the code there is still protected by 'notyet'
    ifdef guard.

    if we would get your diff in, then IMHO the cost of keeping rarely used
    PFRULE_ONCE feature, will further increase. I just don't know if the
    new cost will still be acceptable, therefore I hesitate with my OK for
    your change, which seems to look good otherwise.

thanks and
regards
sashan

Reply via email to