Re: pfctl - allow recursive flush operations [ 5/5 ]

[Alexandr Nedvedicky]

Hello,

</snip>
> > +   unsigned int len;
> strlen(3) returns size_t, malloc(3) takes it.

    makes sense>

> 
> > +   struct pfr_anchors      *anchors;
> > +
> > +   anchors = (struct pfr_anchors *) warg;
> > +
> > +   pfra = malloc(sizeof(*pfra));
> > +   if (pfra == NULL)
> > +           err(1, "%s", __func__);
> > +
> > +   len = strlen(pr->path) + 1;
> > +   len += strlen(pr->name) + 1;
> > +   pfra->pfra_anchorname = malloc(len);
> > +   if (pfra->pfra_anchorname == NULL)
> > +           err(1, "%s", __func__);
> You're always allocating for path and name, but that is too much in the
> else branch.  I think this part can be improved.

    you are right, I've opted for ternary operator when
    calculating len for pr->path.

> 
> 
> > +int
> > +pfctl_recurse(int dev, int opts, const char *anchorname,
> > +    int(*walkf)(int, int, struct pfr_anchoritem *))
> > +{
> > +   int                      rv = 0;
> > +   struct pfr_anchors      *anchors;
> > +   struct pfr_anchoritem   *pfra, *pfra_save;
> > +
> > +   anchors = pfctl_get_anchors(dev, anchorname, opts);
> > +   /*
> > +    * don't let pfctl_clear_*() function to fail with exit
> > +    */
> > +   opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
> Why QUIET as well?

    the pfctl_clear_tables() is not that silent by default.
    we pass pfctl_clear_tables() via walkf argument. I've updated
    the comment above to make explain purpose of PF_OPT_QUIET.

diff below updates patch 5/5.

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index cbf17d01b7d..67dae4cdad9 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2173,7 +2173,7 @@ int
 pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void *warg)
 {
        struct pfr_anchoritem *pfra;
-       unsigned int len;
+       size_t len;
        struct pfr_anchors      *anchors;
 
        anchors = (struct pfr_anchors *) warg;
@@ -2182,7 +2182,7 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void 
*warg)
        if (pfra == NULL)
                err(1, "%s", __func__);
 
-       len = strlen(pr->path) + 1;
+       len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1;
        len += strlen(pr->name) + 1;
        pfra->pfra_anchorname = malloc(len);
        if (pfra->pfra_anchorname == NULL)
@@ -2312,7 +2312,8 @@ pfctl_recurse(int dev, int opts, const char *anchorname,
 
        anchors = pfctl_get_anchors(dev, anchorname, opts);
        /*
-        * don't let pfctl_clear_*() function to fail with exit
+        * don't let pfctl_clear_*() function to fail with exit, also make
+        * pfctl_clear_tables(),(which is passed via walkf argument, quiet.
         */
        opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
        printf("Removing:\n");