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?
Comments inline
> 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 27 Oct 2015 23:24:59 -0000
> @@ -184,6 +184,7 @@ pfr_add_addrs(struct pfr_table *tbl, str
> int *nadd, int flags)
> {
> struct pfioc_table io;
> + int i, rv, add = 0;
>
> if (tbl == NULL || size < 0 || (size && addr == NULL)) {
> errno = EINVAL;
> @@ -192,14 +193,18 @@ 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; /* TODO: check .pfrio_size is needed */
> + for (i = 0; (i < size) && (rv == 0); i++) {
rv is unitialized in the first interation
> + io.pfrio_buffer = addr++;
> + rv = ioctl(dev, DIOCRADDADDR, &io);
I would suggest to return (-1) if ioctl fails...
> + add++;
> + }
> +
> + if ((rv == 0) && (nadd != NULL))
> + *nadd = add;
> +
> + return (rv);
... then you can return (0) here and don't need the rv variable.
> }
>
> 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 27 Oct 2015 23:25:23 -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_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
pfr_add_addr() handles exactly 1 address, don't pass io->pfrio_size.
> break;
> }
>
> Index: sys/net/pf_table.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_table.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 pf_table.c
> --- sys/net/pf_table.c 7 Oct 2015 11:57:44 -0000 1.115
> +++ sys/net/pf_table.c 27 Oct 2015 23:25:26 -0000
> @@ -266,6 +266,54 @@ pfr_clr_addrs(struct pfr_table *tbl, int
> }
>
> int
> +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int
> flags)
Do not pass size.
> +{
> + struct pfr_ktable *kt;
> + struct pfr_kentry *p;
> + struct pfr_addr ad;
> + int rv;
> + time_t tzero = time_second;
> +
> + ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
> + if (pfr_validate_table(tbl, 0, flags & PFR_FLAG_USERIOCTL))
> + return (EINVAL);
> + kt = pfr_lookup_table(tbl);
> + if (kt == NULL || !(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
> + return (ESRCH);
> + if (kt->pfrkt_flags & PFR_TFLAG_CONST)
> + return (EPERM);
> + 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) {
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.
> + }
No { } for one line if block.
> +
> + 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;
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;
> + if (COPYOUT(&ad, addr, sizeof(ad), flags))
> + senderr(EFAULT);
> + }
> +
> + return (0);
> +_bad:
> + if (flags & PFR_FLAG_FEEDBACK)
> + pfr_reset_feedback(addr, size, flags);
Don't use size, it must be 1.
> + return (rv);
rv may be unitialized
> +}
> +
> +int
> pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size,
> int *nadd, int flags)
pfr_add_addrs() is not used anymore, remove it.
> {
> Index: sys/net/pfvar.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pfvar.h,v
> retrieving revision 1.421
> diff -u -p -r1.421 pfvar.h
> --- sys/net/pfvar.h 13 Oct 2015 19:32:32 -0000 1.421
> +++ sys/net/pfvar.h 27 Oct 2015 23:25:29 -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);
> 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 27 Oct 2015 23:27:32 -0000
This file is twice in the diff.
> @@ -184,6 +184,7 @@ pfr_add_addrs(struct pfr_table *tbl, str
> int *nadd, int flags)
> {
> struct pfioc_table io;
> + int i, rv, add = 0;
>
> if (tbl == NULL || size < 0 || (size && addr == NULL)) {
> errno = EINVAL;
> @@ -192,14 +193,18 @@ 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; /* TODO: check .pfrio_size is needed */
> + for (i = 0; (i < size) && (rv == 0); i++) {
> + io.pfrio_buffer = addr++;
> + rv = ioctl(dev, DIOCRADDADDR, &io);
> + add++;
> + }
> +
> + if ((rv == 0) && (nadd != NULL))
> + *nadd = add;
> +
> + return (rv);
> }
>
> int
bluhm
