On Mon, Jan 13, 2020 at 10:47:27PM +0100, Alexandr Nedvedicky wrote:
> @@ -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;
pr->name is always used and only pr->path is optional. With this diff
you're always adding one with the name altough it is only required if
path is given (for the "/" delimiter).
len = strlen(pr->name);
if (pr->path[0])
len += strlen(pr->path) + 1;
This reads simpler and clearer to me, what do you think?
> 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.
Missing space after comma, extraneous parenthese as well.
> */
> opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
Here you add QUIET for all possible walkf() candidates, but only
pfctl_clear_tables() --that is pfctl_table()-- needs it: why not
adjusting this function alone? Does adding QUIET to the table code have
side effects in existing code paths not related to your recursion work?
On the contrary: might there be side effects adding QUIET for all
functions here regardless of whether `-q' or `-v' have been passed on
the command line?
Below is a comment according to style(9) that seems fitting for IGNFAIL
alone (leaving QUIET out now due to above); it's more of a "why" rather
than a "what" so the semantics behind IGNFAIL become clearer since it
isn't used or documented anywhere else.
/*
* While traversing the list, pfctl_clear_*() must always return
* so that failures on one anchor do not prevent clearing others.
*/
opts |= PF_OPT_IGNFAIL;
