Re: pfctl: make functions return void, merge two ifs
On Tue, Jun 13, 2017 at 12:43:51AM +0200, Adam Wolk wrote:
> On Mon, Jun 12, 2017 at 11:43:44PM +0200, Alexandr Nedvedicky wrote:
> > Hello Adam,
> >
> >
> >
> > > It was a rainy evening here, so here's the updated pfctl diff.
> >
> > I'm sorry to hear about the rainy weather [1].
> > anyway, you might want to run regression test for pfctl.
> >
> > cd $SRC/src/regress/sbin/pfctl
> > cat Makefile
> > # follow instructions
> >
> > just for sure.
>
> Ran the tests both on the unmodified and changed pfctl using a stock
> unmodified
> GENERIC kernel.
>
> One test case fails pfcmd1.
>
Yet another rainy day, so revisiting the diff.
1. Re-basing the diff on the recently committed change from the OP
2. Removed the redundant error reporting from:
- pfctl_clear_tables
- pfctl_show_tables
3. pfctl_show_ifaces back to using radix_perror for output consistency
4. regress test altered, we are passing in the -n which just results in the
rules being parsed.
The definition for the pfctlcmd testing group in the Makefile is:
# pfcmd: test pfctl command line parsing
after the change the test failed not because of command line parsing but
from the attempt of trying to clear tables from a non existing anchor.
with the -n flag we still test if the anchor command can be combined
with -Fa but don't actually attempt to run the clearing code.
This regress modification passes both with the pfctl before this change as well
as the newly modified one.
Feedback, OK's?
? openbsd-daily-pfctl-1.diff
? openbsd-daily-pfctl-2.diff
? openbsd-daily-pfctl-3.diff
? parse.c
? pfctl
? rain.diff
Index: pfctl.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl.h,v
retrieving revision 1.53
diff -u -p -r1.53 pfctl.h
--- pfctl.h 19 Jan 2015 23:52:02 - 1.53
+++ pfctl.h 16 Jun 2017 21:05:38 -
@@ -75,12 +75,12 @@ int pfi_get_ifaces(const char *, struct
int pfi_clr_istats(const char *, int *, int);
voidpfctl_print_title(char *);
-int pfctl_clear_tables(const char *, int);
-int pfctl_show_tables(const char *, int);
+voidpfctl_clear_tables(const char *, int);
+voidpfctl_show_tables(const char *, int);
int pfctl_command_tables(int, char *[], char *, const char *, char *,
const char *, int);
voidwarn_namespace_collision(const char *);
-int pfctl_show_ifaces(const char *, int);
+voidpfctl_show_ifaces(const char *, int);
FILE *pfctl_fopen(const char *, const char *);
/*
Index: pfctl_table.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_table.c,v
retrieving revision 1.75
diff -u -p -r1.75 pfctl_table.c
--- pfctl_table.c 13 Apr 2017 07:30:21 - 1.75
+++ pfctl_table.c 16 Jun 2017 21:05:38 -
@@ -102,16 +102,18 @@ static const char *istats_text[2][2][2]
table.pfrt_flags &= ~PFR_TFLAG_PERSIST; \
} while(0)
-int
+void
pfctl_clear_tables(const char *anchor, int opts)
{
- return pfctl_table(0, NULL, NULL, "-F", NULL, anchor, opts);
+ if (pfctl_table(0, NULL, NULL, "-F", NULL, anchor, opts) == -1)
+ exit(1);
}
-int
+void
pfctl_show_tables(const char *anchor, int opts)
{
- return pfctl_table(0, NULL, NULL, "-s", NULL, anchor, opts);
+ if (pfctl_table(0, NULL, NULL, "-s", NULL, anchor, opts) == -1)
+ exit(1);
}
int
@@ -594,7 +596,7 @@ xprintf(int opts, const char *fmt, ...)
/* interface stuff */
-int
+void
pfctl_show_ifaces(const char *filter, int opts)
{
struct pfr_bufferb;
@@ -608,7 +610,7 @@ pfctl_show_ifaces(const char *filter, in
b.pfrb_size = b.pfrb_msize;
if (pfi_get_ifaces(filter, b.pfrb_caddr, _size)) {
radix_perror();
- return (1);
+ exit(1);
}
if (b.pfrb_size <= b.pfrb_msize)
break;
@@ -618,7 +620,6 @@ pfctl_show_ifaces(const char *filter, in
pfctl_print_title("INTERFACES:");
PFRB_FOREACH(p, )
print_iface(p, opts);
- return (0);
}
void
? pfctl-regress-2.diff
? pfctl-regress.diff
Index: pfcmd1.opts
===
RCS file: /cvs/src/regress/sbin/pfctl/pfcmd1.opts,v
retrieving revision 1.1
diff -u -p -r1.1 pfcmd1.opts
--- pfcmd1.opts 3 Jul 2010 02:32:45 - 1.1
+++ pfcmd1.opts 16 Jun 2017 21:34:29 -
@@ -1 +1 @@
--a regress/does_not_exist -Fa
+-n -a regress/does_not_exist -Fa
Re: pfctl: make functions return void, merge two ifs
On Mon, Jun 12, 2017 at 11:43:44PM +0200, Alexandr Nedvedicky wrote: > Hello Adam, > > > > > It was a rainy evening here, so here's the updated pfctl diff. > > I'm sorry to hear about the rainy weather [1]. > anyway, you might want to run regression test for pfctl. > > cd $SRC/src/regress/sbin/pfctl > cat Makefile > # follow instructions > > just for sure. Ran the tests both on the unmodified and changed pfctl using a stock unmodified GENERIC kernel. One test case fails pfcmd1. Passing test redirected to a file: # make > /root/pfctl-old.log cp: /usr/src/regress/sbin/pfctl/pf95.include and /usr/src/regress/sbin/pfctl/pf95.include are identical (not copied). cp: /usr/src/regress/sbin/pfctl/pf103.include and /usr/src/regress/sbin/pfctl/pf103.include are identical (not copied). Loading anchor x from pf103.include rules cleared pfctl: Anchor or Ruleset does not exist. # echo $? 0 # Failing test: # make > /root/pfctl-new.log cp: /usr/src/regress/sbin/pfctl/pf95.include and /usr/src/regress/sbin/pfctl/pf95.include are identical (not copied). cp: /usr/src/regress/sbin/pfctl/pf103.include and /usr/src/regress/sbin/pfctl/pf103.include are identical (not copied). Loading anchor x from pf103.include rules cleared pfctl: Anchor or Ruleset does not exist. pfctl: pfctl_clear_tables: Anchor or Ruleset does not exist *** Error 1 in . (Makefile:238 'pfcmd1') # echo $? 0 # differences in output: # diff -u pfctl-old.log pfctl-new.log --- pfctl-old.log Tue Jun 13 00:07:57 2017 +++ pfctl-new.log Tue Jun 13 00:09:19 2017 @@ -720,6 +720,5 @@ /usr/bin/doas ifconfig tun100 create /usr/bin/doas ifconfig tun101 create /usr/bin/doas pfctl `cat /usr/src/regress/sbin/pfctl/pfcmd1.opts` -f /usr/src/regress/sbin/pfctl/pfcmd1.in -/usr/bin/doas ifconfig lo100 destroy -/usr/bin/doas ifconfig tun100 destroy -/usr/bin/doas ifconfig tun101 destroy +FAILED +*** Error 1 in target 'regress' (ignored) The input data: -a regress/does_not_exist -Fa I did not account for the -a anchor command being able to be combined with other commands. I also ran the regress tests on the original diff sent to the list (without my modifications): # make > /root/pfctl-op.log cp: /usr/src/regress/sbin/pfctl/pf95.include and /usr/src/regress/sbin/pfctl/pf95.include are identical (not copied). cp: /usr/src/regress/sbin/pfctl/pf103.include and /usr/src/regress/sbin/pfctl/pf103.include are identical (not copied). Loading anchor x from pf103.include rules cleared pfctl: Anchor or Ruleset does not exist. # echo $? 0 # differences in output: # diff -u /root/pfctl-old.log /root/pfctl-op.log # They result with no change. > > regards > sasha > > [1] https://www.youtube.com/watch?v=51Kof78YBVM > Thanks, this made my day :)
Re: pfctl: make functions return void, merge two ifs
Hello Adam, > It was a rainy evening here, so here's the updated pfctl diff. I'm sorry to hear about the rainy weather [1]. anyway, you might want to run regression test for pfctl. cd $SRC/src/regress/sbin/pfctl cat Makefile # follow instructions just for sure. regards sasha [1] https://www.youtube.com/watch?v=51Kof78YBVM
Re: pfctl: make functions return void, merge two ifs
On Mon, Jun 12, 2017 at 01:59:07PM +0200, Mike Belopuhov wrote:
> On Sun, Jun 11, 2017 at 15:03 +0100, Raymond wrote:
> > Transform the following functions (which never return anything other than
> > 0, and whose return value is never used) to void:
> >
> > * pfctl_clear_stats, pfctl_clear_interface_flags, pfctl_clear_rules,
> > pfctl_clear_src_nodes, pfctl_clear_states
> > * pfctl_kill_src_nodes, pfctl_net_kill_states, pfctl_label_kill_states,
> > pfctl_id_kill_states, pfctl_key_kill_states
> >
> > inside main: merge two identical if conditions next to each other into one.
> >
> > credit to
> > - awolk@ for the code reading
> > - mikeb for pointing out we can void all _clear_ functions
> > - ghostyy for pointing out all _kill_ functions can be voided
> >
>
> Looks good to me. I was going to point out that pfctl_clear_tables
> should also be converted, but leave that for a rainy day since some
> extra return value checking of pfctl_table call is probably in order.
>
It was a rainy evening here, so here's the updated pfctl diff.
Note that it's based on top of the original diff from this thread.
The additionally modified files are pfctl_table.c and pfctl.h.
Changes:
voided:
- pfctl_clear_tables
- pfctl_show_tables
- pfctl_show_ifaces
Tested on amd64 -current with a kernel modified to output the following
errors for the matching ioctls:
- DIOCRCLRTABLES -> ESRCH
- DIOCRGETTABLES -> ESRCH
- DIOCIGETIFACES -> ENOENT
Attaching the pf_ioctl.diff just for reference.
Looking for feedback, and OK's.
# show interfaces
$ doas pfctl -sI
pfctl: Anchor or Ruleset does not exist.
$ echo $?
0
# show tables
$ doas pfctl -sT
pfctl: Table does not exist.
$ echo $?
0
# clear tables
$ doas pfctl -F Tables
pfctl: Table does not exist.
$ echo $?
0
# show all
$ doas pfctl -sa
... trimmed output ...
pfctl: Table does not exist.
... trimmed output ...
$ echo $?
0
Behavior of the modified pfctl binary
# show interfaces
$ doas ./pfctl -sI
pfctl: pfctl_show_ifaces: Anchor or Ruleset does not exist
$ echo $?
1
# show tables
$ doas ./pfctl -sT
pfctl: Table does not exist.
pfctl: pfctl_show_tables: Table does not exist
$ echo $?
1
# clear tables
$ doas ./pfctl -F Tables
pfctl: Table does not exist.
pfctl: pfctl_show_tables: Table does not exist
$ echo $?
1
# show all
$ doas ./pfctl -sa
pfctl: Table does not exist.
pfctl: pfctl_show_tables: Table does not exist
$ echo $?
1
? openbsd-daily-pfctl-1.diff
? parse.c
? pfctl
? rain.diff
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.344
diff -u -p -r1.344 pfctl.c
--- pfctl.c 30 May 2017 12:13:04 - 1.344
+++ pfctl.c 12 Jun 2017 19:14:27 -
@@ -61,17 +61,17 @@ void usage(void);
int pfctl_enable(int, int);
int pfctl_disable(int, int);
voidpfctl_clear_queues(struct pf_qihead *);
-int pfctl_clear_stats(int, const char *, int);
-int pfctl_clear_interface_flags(int, int);
-int pfctl_clear_rules(int, int, char *);
-int pfctl_clear_src_nodes(int, int);
-int pfctl_clear_states(int, const char *, int);
+voidpfctl_clear_stats(int, const char *, int);
+voidpfctl_clear_interface_flags(int, int);
+voidpfctl_clear_rules(int, int, char *);
+voidpfctl_clear_src_nodes(int, int);
+voidpfctl_clear_states(int, const char *, int);
voidpfctl_addrprefix(char *, struct pf_addr *);
-int pfctl_kill_src_nodes(int, const char *, int);
-int pfctl_net_kill_states(int, const char *, int, int);
-int pfctl_label_kill_states(int, const char *, int, int);
-int pfctl_id_kill_states(int, int);
-int pfctl_key_kill_states(int, const char *, int, int);
+voidpfctl_kill_src_nodes(int, const char *, int);
+voidpfctl_net_kill_states(int, const char *, int, int);
+voidpfctl_label_kill_states(int, const char *, int, int);
+voidpfctl_id_kill_states(int, int);
+voidpfctl_key_kill_states(int, const char *, int, int);
int pfctl_parse_host(char *, struct pf_rule_addr *);
voidpfctl_init_options(struct pfctl *);
int pfctl_load_options(struct pfctl *);
@@ -278,7 +278,7 @@ pfctl_disable(int dev, int opts)
return (0);
}
-int
+void
pfctl_clear_stats(int dev, const char *iface, int opts)
{
struct pfioc_iface pi;
@@ -296,10 +296,9 @@ pfctl_clear_stats(int dev, const char *i
fprintf(stderr, " for interface %s", iface);
fprintf(stderr, "\n");
}
- return (0);
}
-int
+void
pfctl_clear_interface_flags(int dev, int opts)
{
struct pfioc_iface pi;
@@ -313,10 +312,9 @@ pfctl_clear_interface_flags(int dev, int
if ((opts & PF_OPT_QUIET) == 0)
fprintf(stderr, "pf: interface flags reset\n");
}
- return (0);
}
-int
+void
pfctl_clear_rules(int dev, int opts, char *anchorname)
{
struct pfr_buffer t;
@@ -329,20 +327,18 @@
Re: pfctl: make functions return void, merge two ifs
On Sun, Jun 11, 2017 at 15:03 +0100, Raymond wrote:
> Transform the following functions (which never return anything other than 0,
> and whose return value is never used) to void:
>
> * pfctl_clear_stats, pfctl_clear_interface_flags, pfctl_clear_rules,
> pfctl_clear_src_nodes, pfctl_clear_states
> * pfctl_kill_src_nodes, pfctl_net_kill_states, pfctl_label_kill_states,
> pfctl_id_kill_states, pfctl_key_kill_states
>
> inside main: merge two identical if conditions next to each other into one.
>
> credit to
> - awolk@ for the code reading
> - mikeb for pointing out we can void all _clear_ functions
> - ghostyy for pointing out all _kill_ functions can be voided
>
Looks good to me. I was going to point out that pfctl_clear_tables
should also be converted, but leave that for a rainy day since some
extra return value checking of pfctl_table call is probably in order.
> ? parse.c
> ? pfctl
> Index: pfctl.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
> retrieving revision 1.344
> diff -u -p -r1.344 pfctl.c
> --- pfctl.c 30 May 2017 12:13:04 - 1.344
> +++ pfctl.c 11 Jun 2017 13:39:14 -
> @@ -61,17 +61,17 @@ void usage(void);
> int pfctl_enable(int, int);
> int pfctl_disable(int, int);
> void pfctl_clear_queues(struct pf_qihead *);
> -int pfctl_clear_stats(int, const char *, int);
> -int pfctl_clear_interface_flags(int, int);
> -int pfctl_clear_rules(int, int, char *);
> -int pfctl_clear_src_nodes(int, int);
> -int pfctl_clear_states(int, const char *, int);
> +void pfctl_clear_stats(int, const char *, int);
> +void pfctl_clear_interface_flags(int, int);
> +void pfctl_clear_rules(int, int, char *);
> +void pfctl_clear_src_nodes(int, int);
> +void pfctl_clear_states(int, const char *, int);
> void pfctl_addrprefix(char *, struct pf_addr *);
> -int pfctl_kill_src_nodes(int, const char *, int);
> -int pfctl_net_kill_states(int, const char *, int, int);
> -int pfctl_label_kill_states(int, const char *, int, int);
> -int pfctl_id_kill_states(int, int);
> -int pfctl_key_kill_states(int, const char *, int, int);
> +void pfctl_kill_src_nodes(int, const char *, int);
> +void pfctl_net_kill_states(int, const char *, int, int);
> +void pfctl_label_kill_states(int, const char *, int, int);
> +void pfctl_id_kill_states(int, int);
> +void pfctl_key_kill_states(int, const char *, int, int);
> int pfctl_parse_host(char *, struct pf_rule_addr *);
> void pfctl_init_options(struct pfctl *);
> int pfctl_load_options(struct pfctl *);
> @@ -278,7 +278,7 @@ pfctl_disable(int dev, int opts)
> return (0);
> }
>
> -int
> +void
> pfctl_clear_stats(int dev, const char *iface, int opts)
> {
> struct pfioc_iface pi;
> @@ -296,10 +296,9 @@ pfctl_clear_stats(int dev, const char *i
> fprintf(stderr, " for interface %s", iface);
> fprintf(stderr, "\n");
> }
> - return (0);
> }
>
> -int
> +void
> pfctl_clear_interface_flags(int dev, int opts)
> {
> struct pfioc_iface pi;
> @@ -313,10 +312,9 @@ pfctl_clear_interface_flags(int dev, int
> if ((opts & PF_OPT_QUIET) == 0)
> fprintf(stderr, "pf: interface flags reset\n");
> }
> - return (0);
> }
>
> -int
> +void
> pfctl_clear_rules(int dev, int opts, char *anchorname)
> {
> struct pfr_buffer t;
> @@ -329,20 +327,18 @@ pfctl_clear_rules(int dev, int opts, cha
> err(1, "pfctl_clear_rules");
> if ((opts & PF_OPT_QUIET) == 0)
> fprintf(stderr, "rules cleared\n");
> - return (0);
> }
>
> -int
> +void
> pfctl_clear_src_nodes(int dev, int opts)
> {
> if (ioctl(dev, DIOCCLRSRCNODES))
> err(1, "DIOCCLRSRCNODES");
> if ((opts & PF_OPT_QUIET) == 0)
> fprintf(stderr, "source tracking entries cleared\n");
> - return (0);
> }
>
> -int
> +void
> pfctl_clear_states(int dev, const char *iface, int opts)
> {
> struct pfioc_state_kill psk;
> @@ -356,7 +352,6 @@ pfctl_clear_states(int dev, const char *
> err(1, "DIOCCLRSTATES");
> if ((opts & PF_OPT_QUIET) == 0)
> fprintf(stderr, "%d states cleared\n", psk.psk_killed);
> - return (0);
> }
>
> void
> @@ -409,7 +404,7 @@ pfctl_addrprefix(char *addr, struct pf_a
> freeaddrinfo(res);
> }
>
> -int
> +void
> pfctl_kill_src_nodes(int dev, const char *iface, int opts)
> {
> struct pfioc_src_node_kill psnk;
> @@ -509,10 +504,9 @@ pfctl_kill_src_nodes(int dev, const char
> if ((opts & PF_OPT_QUIET) == 0)
> fprintf(stderr, "killed %d src nodes from %d sources and %d "
> "destinations\n", killed, sources, dests);
> - return (0);
> }
>
> -int
> +void
> pfctl_net_kill_states(int dev, const char *iface, int opts, int rdomain)
> {
> struct pfioc_state_kill psk;
> @@ -617,10 +611,9 @@
Re: pfctl: make functions return void, merge two ifs
On Sun, Jun 11, 2017 at 03:03:56PM +0100, Raymond wrote: > Transform the following functions (which never return anything other than 0, > and whose return value is never used) to void: > > * pfctl_clear_stats, pfctl_clear_interface_flags, pfctl_clear_rules, > pfctl_clear_src_nodes, pfctl_clear_states > * pfctl_kill_src_nodes, pfctl_net_kill_states, pfctl_label_kill_states, > pfctl_id_kill_states, pfctl_key_kill_states > > inside main: merge two identical if conditions next to each other into one. > > credit to > - awolk@ for the code reading > - mikeb for pointing out we can void all _clear_ functions > - ghostyy for pointing out all _kill_ functions can be voided > OK awolk@, would like to have an OK from a pf person before committing.
