Hello,
On Tue, Jan 14, 2020 at 03:02:09PM +0100, Klemens Nanni wrote:
> 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?
OK, I'll buy if (...) from you, but '+ 1' must stay there,
because it is for a '\0' terminator. So let's go for this:
len = strlen(pr->name) + 1;
if (pr->path[0])
len += strlen(pr->path) + 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.
> Missing space after comma, extraneous parenthese as well.
>
> > */
> > opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
my comment was misleading. I also want to make pfctl_clear_rules()
silent. But setting PF_OPT_QUIET flag in pfctl_call_() functions,
where we need it makes sense.
</snip>
>
> 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;
comment reads far better.
diff below brings suggested changes to 5/5 patch.
thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 67dae4cdad9..9c5778f4f97 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2182,8 +2182,10 @@ pfctl_walk_get(int opts, struct pfioc_ruleset *pr, void
*warg)
if (pfra == NULL)
err(1, "%s", __func__);
- len = (pr->path[0] == '\0') ? 0 : strlen(pr->path) + 1;
- len += strlen(pr->name) + 1;
+ len = strlen(pr->name) + 1;
+ if (pr->path[0])
+ len += strlen(pr->path) + 1;
+
pfra->pfra_anchorname = malloc(len);
if (pfra->pfra_anchorname == NULL)
err(1, "%s", __func__);
@@ -2281,6 +2283,11 @@ pfctl_get_anchors(int dev, const char *anchor, int opts)
int
pfctl_call_cleartables(int dev, int opts, struct pfr_anchoritem *pfra)
{
+ /*
+ * PF_OPT_QUIET makes pfctl_clear_tables() to stop printing number of
+ * tables cleared for given anchor.
+ */
+ opts |= PF_OPT_QUIET;
return ((pfctl_clear_tables(pfra->pfra_anchorname, opts) == -1) ?
1 : 0);
}
@@ -2288,6 +2295,11 @@ pfctl_call_cleartables(int dev, int opts, struct
pfr_anchoritem *pfra)
int
pfctl_call_clearrules(int dev, int opts, struct pfr_anchoritem *pfra)
{
+ /*
+ * PF_OPT_QUIET makes pfctl_clear_rules() to stop printing a 'rules
+ * cleared' message for every anchor it deletes.
+ */
+ opts |= PF_OPT_QUIET;
return (pfctl_clear_rules(dev, opts, pfra->pfra_anchorname));
}
@@ -2297,7 +2309,7 @@ pfctl_call_clearanchors(int dev, int opts, struct
pfr_anchoritem *pfra)
int rv = 0;
rv |= pfctl_call_cleartables(dev, opts, pfra);
- rv |= pfctl_clear_rules(dev, opts, pfra->pfra_anchorname);
+ rv |= pfctl_call_clearrules(dev, opts, pfra);
return (rv);
}
@@ -2312,10 +2324,10 @@ 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, also make
- * pfctl_clear_tables(),(which is passed via walkf argument, quiet.
+ * While traversing the list, pfctl_clear_*() must always return
+ * so that failures on one anchor do not prevent clearing others.
*/
- opts |= PF_OPT_IGNFAIL | PF_OPT_QUIET;
+ opts |= PF_OPT_IGNFAIL;
printf("Removing:\n");
SLIST_FOREACH_SAFE(pfra, anchors, pfra_sle, pfra_save) {
printf(" %s\n", (*pfra->pfra_anchorname == '\0') ?
