Hello,
> On Wed, Oct 28, 2015 at 06:19:48PM +0100, Alexandr Nedvedicky wrote:
> > The idea has been proposed by Claudio at Varazdin.
>
> I guess the idea is to eliminate the workq. Or is ther naother
> reason to change it?
the primary goal is to kill work queues.
>
> Comments inline
>
thank you very much for very good code review.
> > - *nadd = io.pfrio_nadd;
> > - return (0);
> > + io.pfrio_size = 1; /* TODO: check .pfrio_size is needed */
> > + for (i = 0; (i < size) && (rv == 0); i++) {
>
> rv is unitialized in the first interation
it's fixed by change below:
@@ -184,7 +184,8 @@
int *nadd, int flags)
{
struct pfioc_table io;
- int i, rv, add = 0;
+ int i, add = 0;
+ int rv = 0;
>
> > + io.pfrio_buffer = addr++;
> > + rv = ioctl(dev, DIOCRADDADDR, &io);
>
> I would suggest to return (-1) if ioctl fails...
I'll write my response in email answering your note on 'illusion of atomicity'
>
> pfr_add_addr() handles exactly 1 address, don't pass io->pfrio_size.
>
yes that's true...
error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer,
- io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
+ io->pfrio_flags | PFR_FLAG_USERIOCTL);
break;
> >
> > int
> > +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int
> > flags)
>
> Do not pass size.
-pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int flags)
+pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags)
>
> Should you check for !(flags & PFR_FLAG_DUMMY) here? It was done
> in the old code before pfr_insert_kentries().
>
> > + rv = pfr_insert_kentry(kt, &ad, tzero);
>
> The old code ignored wether pfr_insert_kentries() succeeded.
>
thanks for catching this.
@@ -288,20 +288,20 @@
senderr(EINVAL);
p = pfr_lookup_addr(kt, &ad, 1);
if (p == NULL) {
- rv = pfr_insert_kentry(kt, &ad, tzero);
+ if (!(flags & PFR_FLAG_DUMMY))
+ rv = pfr_insert_kentry(kt, &ad, tzero);
+ else
+ rv = 0;
}
>
> No { } for one line if block.
there is no longer one line if block after fixing PFR_FLAG_DUMMY flag.
>
> PFR_FB_DUPLICATE was used when there were two identical addresses
> in the passed list. This cannot happen anymore. It should be
> PFR_FB_NONE in this case.
>
> > + } else if (rv != 0) {
> > + ad.pfra_fback = PFR_FB_NONE;
> > + } else {
> > + ad.pfra_fback = PFR_FB_ADDED;
> > + }
>
> Perhaps write this block as
>
> if (p == NULL)
> ad.pfra_fback = PFR_FB_ADDED;
> else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not))
> ad.pfra_fback = PFR_FB_CONFLICT;
> else
> ad.pfra_fback = PFR_FB_NONE;
>
I thinks we still must check rv coming from pfr_insert_kentry():
if (flags & PFR_FLAG_FEEDBACK) {
- if (p != NULL) {
- if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)
- ad.pfra_fback = PFR_FB_CONFLICT;
- else
- ad.pfra_fback = PFR_FB_DUPLICATE;
- } else if (rv != 0) {
+ if (p == NULL)
+ ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE;
+ else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)
+ ad.pfra_fback = PFR_FB_CONFLICT;
+ else
ad.pfra_fback = PFR_FB_NONE;
- } else {
- ad.pfra_fback = PFR_FB_ADDED;
- }
> > +_bad:
> > + if (flags & PFR_FLAG_FEEDBACK)
> > + pfr_reset_feedback(addr, size, flags);
>
> Don't use size, it must be 1.
the size argument got dropped.
>
> > + return (rv);
>
> rv may be unitialized
thanks for catching that.
>
> pfr_add_addrs() is not used anymore, remove it.
pfr_add_addrs() is gone in new patch.
thanks and
regards
sasha
--------8<---------------8<-----------------8<--------
Index: sbin/pfctl/pfctl_radix.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
retrieving revision 1.32
diff -u -p -r1.32 pfctl_radix.c
--- sbin/pfctl/pfctl_radix.c 21 Jan 2015 21:50:33 -0000 1.32
+++ sbin/pfctl/pfctl_radix.c 9 Nov 2015 20:43:52 -0000
@@ -184,6 +184,8 @@ pfr_add_addrs(struct pfr_table *tbl, str
int *nadd, int flags)
{
struct pfioc_table io;
+ int i, add = 0;
+ int rv = 0;
if (tbl == NULL || size < 0 || (size && addr == NULL)) {
errno = EINVAL;
@@ -192,14 +194,19 @@ pfr_add_addrs(struct pfr_table *tbl, str
bzero(&io, sizeof io);
io.pfrio_flags = flags;
io.pfrio_table = *tbl;
- io.pfrio_buffer = addr;
io.pfrio_esize = sizeof(*addr);
- io.pfrio_size = size;
- if (ioctl(dev, DIOCRADDADDRS, &io))
- return (-1);
- if (nadd != NULL)
- *nadd = io.pfrio_nadd;
- return (0);
+ io.pfrio_size = 1;
+ for (i = 0; (i < size) && (rv == 0); i++) {
+ io.pfrio_buffer = addr++;
+ if (ioctl(dev, DIOCRADDADDR, &io) == -1)
+ rv = -1;
+ add++;
+ }
+
+ if ((rv == 0) && (nadd != NULL))
+ *nadd = add;
+
+ return (rv);
}
int
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.291
diff -u -p -r1.291 pf_ioctl.c
--- sys/net/pf_ioctl.c 13 Oct 2015 19:32:31 -0000 1.291
+++ sys/net/pf_ioctl.c 9 Nov 2015 20:44:21 -0000
@@ -834,7 +834,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCRGETTSTATS:
case DIOCRCLRTSTATS:
case DIOCRCLRADDRS:
- case DIOCRADDADDRS:
+ case DIOCRADDADDR:
case DIOCRDELADDRS:
case DIOCRSETADDRS:
case DIOCRGETASTATS:
@@ -887,7 +887,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCRDELTABLES:
case DIOCRCLRTSTATS:
case DIOCRCLRADDRS:
- case DIOCRADDADDRS:
+ case DIOCRADDADDR:
case DIOCRDELADDRS:
case DIOCRSETADDRS:
case DIOCRSETTFLAGS:
@@ -1816,16 +1816,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
break;
}
- case DIOCRADDADDRS: {
+ case DIOCRADDADDR: {
struct pfioc_table *io = (struct pfioc_table *)addr;
if (io->pfrio_esize != sizeof(struct pfr_addr)) {
error = ENODEV;
break;
}
- error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer,
- io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags |
- PFR_FLAG_USERIOCTL);
+ error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer,
+ io->pfrio_flags | PFR_FLAG_USERIOCTL);
break;
}
Index: sys/net/pf_table.c
===================================================================
RCS file: /cvs/src/sys/net/pf_table.c,v
retrieving revision 1.116
diff -u -p -r1.116 pf_table.c
--- sys/net/pf_table.c 3 Nov 2015 22:10:33 -0000 1.116
+++ sys/net/pf_table.c 9 Nov 2015 20:44:23 -0000
@@ -266,14 +266,12 @@ pfr_clr_addrs(struct pfr_table *tbl, int
}
int
-pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size,
- int *nadd, int flags)
+pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags)
{
- struct pfr_ktable *kt, *tmpkt;
- struct pfr_kentryworkq workq;
- struct pfr_kentry *p, *q;
+ struct pfr_ktable *kt;
+ struct pfr_kentry *p;
struct pfr_addr ad;
- int i, rv, xadd = 0;
+ int rv = 0;
time_t tzero = time_second;
ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
@@ -284,61 +282,34 @@ pfr_add_addrs(struct pfr_table *tbl, str
return (ESRCH);
if (kt->pfrkt_flags & PFR_TFLAG_CONST)
return (EPERM);
- tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0,
- !(flags & PFR_FLAG_USERIOCTL));
- if (tmpkt == NULL)
- return (ENOMEM);
- SLIST_INIT(&workq);
- for (i = 0; i < size; i++) {
- YIELD(i, flags & PFR_FLAG_USERIOCTL);
- if (COPYIN(addr+i, &ad, sizeof(ad), flags))
+ if (COPYIN(addr, &ad, sizeof(ad), flags))
+ senderr(EFAULT);
+ if (pfr_validate_addr(&ad))
+ senderr(EINVAL);
+ p = pfr_lookup_addr(kt, &ad, 1);
+ if (p == NULL) {
+ if (!(flags & PFR_FLAG_DUMMY))
+ rv = pfr_insert_kentry(kt, &ad, tzero);
+ else
+ rv = 0;
+ }
+
+ if (flags & PFR_FLAG_FEEDBACK) {
+ if (p == NULL)
+ ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE;
+ else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)
+ ad.pfra_fback = PFR_FB_CONFLICT;
+ else
+ ad.pfra_fback = PFR_FB_NONE;
+
+ if (COPYOUT(&ad, addr, sizeof(ad), flags))
senderr(EFAULT);
- if (pfr_validate_addr(&ad))
- senderr(EINVAL);
- p = pfr_lookup_addr(kt, &ad, 1);
- q = pfr_lookup_addr(tmpkt, &ad, 1);
- if (flags & PFR_FLAG_FEEDBACK) {
- if (q != NULL)
- ad.pfra_fback = PFR_FB_DUPLICATE;
- else if (p == NULL)
- ad.pfra_fback = PFR_FB_ADDED;
- else if ((p->pfrke_flags & PFRKE_FLAG_NOT) !=
- ad.pfra_not)
- ad.pfra_fback = PFR_FB_CONFLICT;
- else
- ad.pfra_fback = PFR_FB_NONE;
- }
- if (p == NULL && q == NULL) {
- p = pfr_create_kentry(&ad);
- if (p == NULL)
- senderr(ENOMEM);
- if (pfr_route_kentry(tmpkt, p)) {
- pfr_destroy_kentry(p);
- ad.pfra_fback = PFR_FB_NONE;
- } else {
- SLIST_INSERT_HEAD(&workq, p, pfrke_workq);
- xadd++;
- }
- }
- if (flags & PFR_FLAG_FEEDBACK)
- if (COPYOUT(&ad, addr+i, sizeof(ad), flags))
- senderr(EFAULT);
}
- pfr_clean_node_mask(tmpkt, &workq);
- if (!(flags & PFR_FLAG_DUMMY)) {
- pfr_insert_kentries(kt, &workq, tzero);
- } else
- pfr_destroy_kentries(&workq);
- if (nadd != NULL)
- *nadd = xadd;
- pfr_destroy_ktable(tmpkt, 0);
+
return (0);
_bad:
- pfr_clean_node_mask(tmpkt, &workq);
- pfr_destroy_kentries(&workq);
if (flags & PFR_FLAG_FEEDBACK)
- pfr_reset_feedback(addr, size, flags);
- pfr_destroy_ktable(tmpkt, 0);
+ pfr_reset_feedback(addr, 1, flags);
return (rv);
}
Index: sys/net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.422
diff -u -p -r1.422 pfvar.h
--- sys/net/pfvar.h 30 Oct 2015 11:33:55 -0000 1.422
+++ sys/net/pfvar.h 9 Nov 2015 20:44:24 -0000
@@ -1613,7 +1613,7 @@ struct pfioc_iface {
#define DIOCRGETTSTATS _IOWR('D', 64, struct pfioc_table)
#define DIOCRCLRTSTATS _IOWR('D', 65, struct pfioc_table)
#define DIOCRCLRADDRS _IOWR('D', 66, struct pfioc_table)
-#define DIOCRADDADDRS _IOWR('D', 67, struct pfioc_table)
+#define DIOCRADDADDR _IOWR('D', 67, struct pfioc_table)
#define DIOCRDELADDRS _IOWR('D', 68, struct pfioc_table)
#define DIOCRSETADDRS _IOWR('D', 69, struct pfioc_table)
#define DIOCRGETADDRS _IOWR('D', 70, struct pfioc_table)
@@ -1789,8 +1789,7 @@ int pfr_clr_tstats(struct pfr_table *, i
int pfr_set_tflags(struct pfr_table *, int, int, int, int *, int *, int);
int pfr_clr_addrs(struct pfr_table *, int *, int);
int pfr_insert_kentry(struct pfr_ktable *, struct pfr_addr *, time_t);
-int pfr_add_addrs(struct pfr_table *, struct pfr_addr *, int, int *,
- int);
+int pfr_add_addr(struct pfr_table *, struct pfr_addr *, int);
int pfr_del_addrs(struct pfr_table *, struct pfr_addr *, int, int *,
int);
int pfr_set_addrs(struct pfr_table *, struct pfr_addr *, int, int *,
Index: sbin/pfctl/pfctl_radix.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
retrieving revision 1.32
diff -u -p -r1.32 pfctl_radix.c
--- sbin/pfctl/pfctl_radix.c 21 Jan 2015 21:50:33 -0000 1.32
+++ sbin/pfctl/pfctl_radix.c 9 Nov 2015 20:46:39 -0000
@@ -184,6 +184,8 @@ pfr_add_addrs(struct pfr_table *tbl, str
int *nadd, int flags)
{
struct pfioc_table io;
+ int i, add = 0;
+ int rv = 0;
if (tbl == NULL || size < 0 || (size && addr == NULL)) {
errno = EINVAL;
@@ -192,14 +194,19 @@ pfr_add_addrs(struct pfr_table *tbl, str
bzero(&io, sizeof io);
io.pfrio_flags = flags;
io.pfrio_table = *tbl;
- io.pfrio_buffer = addr;
io.pfrio_esize = sizeof(*addr);
- io.pfrio_size = size;
- if (ioctl(dev, DIOCRADDADDRS, &io))
- return (-1);
- if (nadd != NULL)
- *nadd = io.pfrio_nadd;
- return (0);
+ io.pfrio_size = 1;
+ for (i = 0; (i < size) && (rv == 0); i++) {
+ io.pfrio_buffer = addr++;
+ if (ioctl(dev, DIOCRADDADDR, &io) == -1)
+ rv = -1;
+ add++;
+ }
+
+ if ((rv == 0) && (nadd != NULL))
+ *nadd = add;
+
+ return (rv);
}
int
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.291
diff -u -p -r1.291 pf_ioctl.c
--- sys/net/pf_ioctl.c 13 Oct 2015 19:32:31 -0000 1.291
+++ sys/net/pf_ioctl.c 9 Nov 2015 20:47:09 -0000
@@ -834,7 +834,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCRGETTSTATS:
case DIOCRCLRTSTATS:
case DIOCRCLRADDRS:
- case DIOCRADDADDRS:
+ case DIOCRADDADDR:
case DIOCRDELADDRS:
case DIOCRSETADDRS:
case DIOCRGETASTATS:
@@ -887,7 +887,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCRDELTABLES:
case DIOCRCLRTSTATS:
case DIOCRCLRADDRS:
- case DIOCRADDADDRS:
+ case DIOCRADDADDR:
case DIOCRDELADDRS:
case DIOCRSETADDRS:
case DIOCRSETTFLAGS:
@@ -1816,16 +1816,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
break;
}
- case DIOCRADDADDRS: {
+ case DIOCRADDADDR: {
struct pfioc_table *io = (struct pfioc_table *)addr;
if (io->pfrio_esize != sizeof(struct pfr_addr)) {
error = ENODEV;
break;
}
- error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer,
- io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags |
- PFR_FLAG_USERIOCTL);
+ error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer,
+ io->pfrio_flags | PFR_FLAG_USERIOCTL);
break;
}
Index: sys/net/pf_table.c
===================================================================
RCS file: /cvs/src/sys/net/pf_table.c,v
retrieving revision 1.116
diff -u -p -r1.116 pf_table.c
--- sys/net/pf_table.c 3 Nov 2015 22:10:33 -0000 1.116
+++ sys/net/pf_table.c 9 Nov 2015 20:47:10 -0000
@@ -266,14 +266,12 @@ pfr_clr_addrs(struct pfr_table *tbl, int
}
int
-pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size,
- int *nadd, int flags)
+pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags)
{
- struct pfr_ktable *kt, *tmpkt;
- struct pfr_kentryworkq workq;
- struct pfr_kentry *p, *q;
+ struct pfr_ktable *kt;
+ struct pfr_kentry *p;
struct pfr_addr ad;
- int i, rv, xadd = 0;
+ int rv = 0;
time_t tzero = time_second;
ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
@@ -284,61 +282,34 @@ pfr_add_addrs(struct pfr_table *tbl, str
return (ESRCH);
if (kt->pfrkt_flags & PFR_TFLAG_CONST)
return (EPERM);
- tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0,
- !(flags & PFR_FLAG_USERIOCTL));
- if (tmpkt == NULL)
- return (ENOMEM);
- SLIST_INIT(&workq);
- for (i = 0; i < size; i++) {
- YIELD(i, flags & PFR_FLAG_USERIOCTL);
- if (COPYIN(addr+i, &ad, sizeof(ad), flags))
+ if (COPYIN(addr, &ad, sizeof(ad), flags))
+ senderr(EFAULT);
+ if (pfr_validate_addr(&ad))
+ senderr(EINVAL);
+ p = pfr_lookup_addr(kt, &ad, 1);
+ if (p == NULL) {
+ if (!(flags & PFR_FLAG_DUMMY))
+ rv = pfr_insert_kentry(kt, &ad, tzero);
+ else
+ rv = 0;
+ }
+
+ if (flags & PFR_FLAG_FEEDBACK) {
+ if (p == NULL)
+ ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE;
+ else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)
+ ad.pfra_fback = PFR_FB_CONFLICT;
+ else
+ ad.pfra_fback = PFR_FB_NONE;
+
+ if (COPYOUT(&ad, addr, sizeof(ad), flags))
senderr(EFAULT);
- if (pfr_validate_addr(&ad))
- senderr(EINVAL);
- p = pfr_lookup_addr(kt, &ad, 1);
- q = pfr_lookup_addr(tmpkt, &ad, 1);
- if (flags & PFR_FLAG_FEEDBACK) {
- if (q != NULL)
- ad.pfra_fback = PFR_FB_DUPLICATE;
- else if (p == NULL)
- ad.pfra_fback = PFR_FB_ADDED;
- else if ((p->pfrke_flags & PFRKE_FLAG_NOT) !=
- ad.pfra_not)
- ad.pfra_fback = PFR_FB_CONFLICT;
- else
- ad.pfra_fback = PFR_FB_NONE;
- }
- if (p == NULL && q == NULL) {
- p = pfr_create_kentry(&ad);
- if (p == NULL)
- senderr(ENOMEM);
- if (pfr_route_kentry(tmpkt, p)) {
- pfr_destroy_kentry(p);
- ad.pfra_fback = PFR_FB_NONE;
- } else {
- SLIST_INSERT_HEAD(&workq, p, pfrke_workq);
- xadd++;
- }
- }
- if (flags & PFR_FLAG_FEEDBACK)
- if (COPYOUT(&ad, addr+i, sizeof(ad), flags))
- senderr(EFAULT);
}
- pfr_clean_node_mask(tmpkt, &workq);
- if (!(flags & PFR_FLAG_DUMMY)) {
- pfr_insert_kentries(kt, &workq, tzero);
- } else
- pfr_destroy_kentries(&workq);
- if (nadd != NULL)
- *nadd = xadd;
- pfr_destroy_ktable(tmpkt, 0);
+
return (0);
_bad:
- pfr_clean_node_mask(tmpkt, &workq);
- pfr_destroy_kentries(&workq);
if (flags & PFR_FLAG_FEEDBACK)
- pfr_reset_feedback(addr, size, flags);
- pfr_destroy_ktable(tmpkt, 0);
+ pfr_reset_feedback(addr, 1, flags);
return (rv);
}
Index: sys/net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.422
diff -u -p -r1.422 pfvar.h
--- sys/net/pfvar.h 30 Oct 2015 11:33:55 -0000 1.422
+++ sys/net/pfvar.h 9 Nov 2015 20:47:12 -0000
@@ -1613,7 +1613,7 @@ struct pfioc_iface {
#define DIOCRGETTSTATS _IOWR('D', 64, struct pfioc_table)
#define DIOCRCLRTSTATS _IOWR('D', 65, struct pfioc_table)
#define DIOCRCLRADDRS _IOWR('D', 66, struct pfioc_table)
-#define DIOCRADDADDRS _IOWR('D', 67, struct pfioc_table)
+#define DIOCRADDADDR _IOWR('D', 67, struct pfioc_table)
#define DIOCRDELADDRS _IOWR('D', 68, struct pfioc_table)
#define DIOCRSETADDRS _IOWR('D', 69, struct pfioc_table)
#define DIOCRGETADDRS _IOWR('D', 70, struct pfioc_table)
@@ -1789,8 +1789,7 @@ int pfr_clr_tstats(struct pfr_table *, i
int pfr_set_tflags(struct pfr_table *, int, int, int, int *, int *, int);
int pfr_clr_addrs(struct pfr_table *, int *, int);
int pfr_insert_kentry(struct pfr_ktable *, struct pfr_addr *, time_t);
-int pfr_add_addrs(struct pfr_table *, struct pfr_addr *, int, int *,
- int);
+int pfr_add_addr(struct pfr_table *, struct pfr_addr *, int);
int pfr_del_addrs(struct pfr_table *, struct pfr_addr *, int, int *,
int);
int pfr_set_addrs(struct pfr_table *, struct pfr_addr *, int, int *,
