On Mon, May 09, 2022 at 11:11:03PM +0200, Alexandr Nedvedicky wrote:
> > ... and then we insert a destroyed p
>
> yes. you are right. new diff addresses that with change as follows:
>
> @@ -1542,9 +1542,8 @@ pfr_add_tables(struct pfr_table ...)
> pfr_destroy_ktable(p, 0);
> break;
> }
> + SLIST_INSERT_HEAD(&auxq, p, pfrkt_workq);
This inserts p each time you run over q list.
> }
> -
> - SLIST_INSERT_HEAD(&auxq, p, pfrkt_workq);
> }
Should we do it like this? It is similar to your solution at the
other loop.
SLIST_FOREACH(q, &auxq, pfrkt_workq) {
if (!pfr_ktable_compare(p, q)) {
/*
* We need no lock here, because `p` is empty,
* there are no rules or shadow tables
* attached.
*/
pfr_destroy_ktable(p->pfrkt_root, 0);
p->pfrkt_root = NULL;
pfr_destroy_ktable(p, 0);
break;
}
}
if (q != NULL)
continue;
SLIST_INSERT_HEAD(&auxq, p, pfrkt_workq);
> > I compared the old and new code to see if it is equivalent.
> > Before the condtion looked like this.
>
> very good point. I think this what needs to be done:
>
> @@ -1558,7 +1558,8 @@ pfr_add_tables(struct pfr_table *tbl, ...)
> if (p == NULL) {
> SLIST_REMOVE(&auxq, n, pfr_ktable, pfrkt_workq);
> SLIST_INSERT_HEAD(&addq, n, pfrkt_workq);
> - } else if (!(flags & PFR_FLAG_DUMMY)) {
> + } else if (!(flags & PFR_FLAG_DUMMY) &&
I guess PFR_FLAG_DUMMY check is an optimization.
> + !(p->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
Indent should be one tab to the left.
> p->pfrkt_nflags = (p->pfrkt_flags &
> ~PFR_TFLAG_USRMASK) | key.pfrkt_flags;
> SLIST_INSERT_HEAD(&changeq, p, pfrkt_workq);
Old code had this to avoid duplicate entries. Do we need it?
SLIST_FOREACH(q, &changeq, pfrkt_workq)
if (!pfr_ktable_compare(&key, q))
goto _skip;
> > This continue goes to the r list, but I think you want to continue p list.
> > > }
> > > }
>
> yes, exactly. we want to continue with outer loop if we break from inner
> one. this is what I want to do:
>
> @@ -1617,9 +1618,12 @@ pfr_add_tables(struct pfr_table *tbl, ...)
> p->pfrkt_root = r;
> SLIST_INSERT_HEAD(&auxq, q,
> pfrkt_workq);
> - continue;
> + break;
> }
> }
> + if (r != SLIST_END(&addq))
Could you use if (r != NULL) ? Noone uses SLIST_END macros.
> + continue;
> +
bluhm
