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