Hello Klemens,
thanks for taking a look at it.
</snip>
On Sun, Nov 20, 2022 at 06:22:53PM +0000, Klemens Nanni wrote:
> >
> > In order to test diff below I create 64 groups for vio0 interface:
> >
> > for i in `seq 64` ; do ifconfig vio0 group g$i\g ; done
> >
> > then I use pfctl -sI to display them. with diff below things do work:
> >
> > netlock# pfctl -sI|wc -l
> > 72
> > netlock#
>
> You might as well turn that into a new regress test.
will do that in follow up commit to regress.
</snip>
> > --- a/sys/net/pf_if.c
> > +++ b/sys/net/pf_if.c
> > @@ -766,12 +766,13 @@ pfi_get_ifaces(const char *name, struct pfi_kif *buf,
> > int *size)
> > nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
> > if (pfi_skip_if(name, p))
> > continue;
> > - if (*size > n++) {
> > + if (*size > ++n) {
>
> You can save the else and one level of indent by doing
> if (*size <= ++n)
> break;
> ...
>
I like it. thanks.
> > if (!p->pfik_tzero)
> > p->pfik_tzero = gettime();
> > memcpy(buf++, p, sizeof(*buf));
> > nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
>
> This duplicate nextp assignment seems useless.
>
> It's already pointing at the next entry as done in the first line of the
> for loop...
>
> which might as well be a simpler RB_FOREACH, avoiding nextp completely.
I was also thinking about it, but decided to stay focused on
fix itself. Feel free to go for it as a follow up commit.
I have no objection to use RB_FOREACH() here.
updated diff below comes with simplified version of pfi_get_ifaces() as
suggested by Klemens.
thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
index 5c0c32e5961..7966fe9ac51 100644
--- a/sbin/pfctl/pfctl_table.c
+++ b/sbin/pfctl/pfctl_table.c
@@ -583,18 +583,16 @@ pfctl_show_ifaces(const char *filter, int opts)
{
struct pfr_buffer b;
struct pfi_kif *p;
- int i = 0;
bzero(&b, sizeof(b));
b.pfrb_type = PFRB_IFACES;
for (;;) {
- pfr_buf_grow(&b, b.pfrb_size);
+ pfr_buf_grow(&b, 0);
b.pfrb_size = b.pfrb_msize;
if (pfi_get_ifaces(filter, b.pfrb_caddr, &b.pfrb_size))
errx(1, "%s", pf_strerror(errno));
- if (b.pfrb_size <= b.pfrb_msize)
+ if (b.pfrb_size < b.pfrb_msize)
break;
- i++;
}
if (opts & PF_OPT_SHOWALL)
pfctl_print_title("INTERFACES:");
diff --git a/sys/net/pf_if.c b/sys/net/pf_if.c
index e23c14e6769..0b5d456bb32 100644
--- a/sys/net/pf_if.c
+++ b/sys/net/pf_if.c
@@ -766,12 +766,12 @@ pfi_get_ifaces(const char *name, struct pfi_kif *buf, int
*size)
nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
if (pfi_skip_if(name, p))
continue;
- if (*size > n++) {
- if (!p->pfik_tzero)
- p->pfik_tzero = gettime();
- memcpy(buf++, p, sizeof(*buf));
- nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
- }
+ if (*size <= ++n)
+ break;
+ if (!p->pfik_tzero)
+ p->pfik_tzero = gettime();
+ memcpy(buf++, p, sizeof(*buf));
+ nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
}
*size = n;
}
