Hello,
> > updated diff is attached.
>
> One comment below but this diff is OK claudio@
>
> > thanks and
> > regards
> > sashan
> >
> > --------8<---------------8<---------------8<------------------8<--------
> > void
> > pfi_xcommit(void)
> > {
> > - struct pfi_kif *p;
> > + struct pfi_kif *p, *gkif;
> > + struct ifg_list *g;
> > + struct ifnet *ifp;
> > + size_t n;
> >
> > - RB_FOREACH(p, pfi_ifhead, &pfi_ifs)
> > + RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
> > p->pfik_flags = p->pfik_flags_new;
> > + n = strlen(p->pfik_name);
> > + ifp = p->pfik_ifp;
> > + /*
> > + * if kif is backed by existing interface, then we must use
> > + * skip flags found in groups. We use pfik_flags_new, otherwise
> > + * we would need to do two RB_FOREACH() passes: the first to
> > + * commit group changes the second to commit flag changes for
> > + * interfaces.
> > + */
> > + if (isdigit(p->pfik_name[n - 1]) && ifp != NULL)
>
> No other code in pf_if.c is checking both pfik_name and ifp != NULL in
> similar situations. I think you should skip the isdigit() check here.
> If there is a real interface connected to a pfi_kif than it should be updated.
> Lets not introduce more complexity here.
>
> > + TAILQ_FOREACH(g, &ifp->if_groups, ifgl_next) {
> > + gkif =
> > + (struct pfi_kif *)g->ifgl_group->ifg_pf_kif;
> > + KASSERT(gkif != NULL);
> > + p->pfik_flags |= gkif->pfik_flags_new;
> > + }
> > + }
yes, there is no reason to keep isdigit() check. if `p` happens to
represent interface group, then ifp is NULL (p->pfik_ifp == NULL
for interface groups).
thanks and
regards
sashan
