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 -0000 1.344
+++ pfctl.c 12 Jun 2017 19:14:27 -0000
@@ -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 @@ pfctl_net_kill_states(int dev, const cha
if ((opts & PF_OPT_QUIET) == 0)
fprintf(stderr, "killed %d states from %d sources and %d "
"destinations\n", killed, sources, dests);
- return (0);
}
-int
+void
pfctl_label_kill_states(int dev, const char *iface, int opts, int rdomain)
{
struct pfioc_state_kill psk;
@@ -645,11 +638,9 @@ pfctl_label_kill_states(int dev, const c
if ((opts & PF_OPT_QUIET) == 0)
fprintf(stderr, "killed %d states\n", psk.psk_killed);
-
- return (0);
}
-int
+void
pfctl_id_kill_states(int dev, int opts)
{
struct pfioc_state_kill psk;
@@ -680,11 +671,9 @@ pfctl_id_kill_states(int dev, int opts)
if ((opts & PF_OPT_QUIET) == 0)
fprintf(stderr, "killed %d states\n", psk.psk_killed);
-
- return (0);
}
-int
+void
pfctl_key_kill_states(int dev, const char *iface, int opts, int rdomain)
{
struct pfioc_state_kill psk;
@@ -741,8 +730,6 @@ pfctl_key_kill_states(int dev, const cha
if ((opts & PF_OPT_QUIET) == 0)
fprintf(stderr, "killed %d states\n", psk.psk_killed);
-
- return (0);
}
int
@@ -2558,13 +2545,11 @@ main(int argc, char *argv[])
}
}
- if ((rulesopt != NULL) && !anchorname[0])
- if (pfctl_clear_interface_flags(dev, opts | PF_OPT_QUIET))
- error = 1;
-
- if (rulesopt != NULL && !anchorname[0])
+ if (rulesopt != NULL && !anchorname[0]) {
+ pfctl_clear_interface_flags(dev, opts | PF_OPT_QUIET);
if (pfctl_file_fingerprints(dev, opts, PF_OSFP_FILE))
error = 1;
+ }
if (rulesopt != NULL) {
if (anchorname[0] == '_' || strstr(anchorname, "/_") != NULL)
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 -0000 1.53
+++ pfctl.h 12 Jun 2017 19:14:27 -0000
@@ -75,12 +75,12 @@ int pfi_get_ifaces(const char *, struct
int pfi_clr_istats(const char *, int *, int);
void pfctl_print_title(char *);
-int pfctl_clear_tables(const char *, int);
-int pfctl_show_tables(const char *, int);
+void pfctl_clear_tables(const char *, int);
+void pfctl_show_tables(const char *, int);
int pfctl_command_tables(int, char *[], char *, const char *, char *,
const char *, int);
void warn_namespace_collision(const char *);
-int pfctl_show_ifaces(const char *, int);
+void pfctl_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 -0000 1.75
+++ pfctl_table.c 12 Jun 2017 19:14:27 -0000
@@ -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)
+ errx(1, "pfctl_clear_tables: %s", pfr_strerror(errno));
}
-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)
+ errx(1, "pfctl_show_tables: %s", pfr_strerror(errno));
}
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_buffer b;
@@ -606,10 +608,8 @@ pfctl_show_ifaces(const char *filter, in
for (;;) {
pfr_buf_grow(&b, b.pfrb_size);
b.pfrb_size = b.pfrb_msize;
- if (pfi_get_ifaces(filter, b.pfrb_caddr, &b.pfrb_size)) {
- radix_perror();
- return (1);
- }
+ if (pfi_get_ifaces(filter, b.pfrb_caddr, &b.pfrb_size))
+ errx(1, "pfctl_show_ifaces: %s", pfr_strerror(errno));
if (b.pfrb_size <= b.pfrb_msize)
break;
i++;
@@ -618,7 +618,6 @@ pfctl_show_ifaces(const char *filter, in
pfctl_print_title("INTERFACES:");
PFRB_FOREACH(p, &b)
print_iface(p, opts);
- return (0);
}
void
Index: pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.315
diff -u -p -r1.315 pf_ioctl.c
--- pf_ioctl.c 5 Jun 2017 22:18:28 -0000 1.315
+++ pf_ioctl.c 12 Jun 2017 20:49:37 -0000
@@ -1866,6 +1866,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
error = pfr_clr_tables(&io->pfrio_table, &io->pfrio_ndel,
io->pfrio_flags | PFR_FLAG_USERIOCTL);
+ error = ESRCH;
break;
}
@@ -1902,6 +1903,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
error = pfr_get_tables(&io->pfrio_table, io->pfrio_buffer,
&io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
+ error = ESRCH;
break;
}
@@ -2412,6 +2414,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
}
error = pfi_get_ifaces(io->pfiio_name, io->pfiio_buffer,
&io->pfiio_size);
+ error = ENOENT;
break;
}
