Alexander Bluhm <[email protected]> writes:
>> I like this too. I somehow got the impression that macros are severely
>> frowned upon and didn't offer this kind of interface before.
>>
>> If you get this submitted, I can do a pass through the codebase to be
>> sure we catch them all.
Vitaliy, I volunteer to do a separate pass, no reason to gate this clear
improvement.
Q: would the introduction of SYSCTL_BOOL be considered an improvement
over "0, 1"?
> Anyone who wants to OK it?
OK gnezdo, nits inline.
>
> bluhm
>
> Index: kern/kern_sysctl.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.390
> diff -u -p -r1.390 kern_sysctl.c
> --- kern/kern_sysctl.c 23 Apr 2021 07:21:02 -0000 1.390
> +++ kern/kern_sysctl.c 28 Apr 2021 10:04:02 -0000
> @@ -818,7 +818,8 @@ debug_sysctl(int *name, u_int namelen, v
> * Reads, or writes that lower the value
> */
> int
> -sysctl_int_lower(void *oldp, size_t *oldlenp, void *newp, size_t newlen, int
> *valp)
> +sysctl_int_lower(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
> + int *valp)
> {
> unsigned int oval = *valp, val = *valp;
> int error;
> @@ -841,35 +842,40 @@ sysctl_int_lower(void *oldp, size_t *old
> int
> sysctl_int(void *oldp, size_t *oldlenp, void *newp, size_t newlen, int *valp)
> {
> - return (sysctl_int_bounded(oldp, oldlenp, newp, newlen, valp, 0, 0));
> -}
> -
> -int
> -sysctl_int_bounded(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
> - int *valp, int minimum, int maximum)
> -{
> int error = 0;
> - int val;
>
> if (oldp && *oldlenp < sizeof(int))
> return (ENOMEM);
> if (newp && newlen != sizeof(int))
> return (EINVAL);
> *oldlenp = sizeof(int);
> - val = *valp;
> if (oldp)
> - error = copyout(&val, oldp, sizeof(int));
> + error = copyout(valp, oldp, sizeof(int));
> if (error == 0 && newp)
> - error = copyin(newp, &val, sizeof(int));
> - if (error)
> - return (error);
> - if (minimum == maximum || (minimum <= val && val <= maximum))
> - *valp = val;
> - else
> - error = EINVAL;
> + error = copyin(newp, valp, sizeof(int));
> return (error);
> }
>
> +int
> +sysctl_int_bounded(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
> + int *valp, int minimum, int maximum)
> +{
> + int val = *valp;
> + int error;
> +
> + /* read only */
> + if (newp == NULL || minimum > maximum)
> + return (sysctl_rdint(oldp, oldlenp, newp, *valp));
+ return (sysctl_rdint(oldp, oldlenp, newp, val));
This would make valp be used exactly to read and write the
variable. Seems a tiny bit simpler.
> +
> + if ((error = sysctl_int(oldp, oldlenp, newp, newlen, &val)))
> + return (error);
> + /* bounded and not within limits */
+ /* bounded and outside limits */
Seems a tiny bit more direct and aligned with the code which has no
negations.
> + if (minimum < maximum && (val < minimum || maximum < val))
> + return (EINVAL);
> + *valp = val;
> + return (0);
> +}
> +
> /*
> * As above, but read-only.
> */
> @@ -901,14 +907,8 @@ sysctl_bounded_arr(const struct sysctl_b
> return (ENOTDIR);
> for (i = 0; i < valplen; ++i) {
> if (valpp[i].mib == name[0]) {
> - if (valpp[i].minimum <= valpp[i].maximum) {
> - return (sysctl_int_bounded(oldp, oldlenp, newp,
> - newlen, valpp[i].var, valpp[i].minimum,
> - valpp[i].maximum));
> - } else {
> - return (sysctl_rdint(oldp, oldlenp, newp,
> - *valpp[i].var));
> - }
> + return (sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> + valpp[i].var, valpp[i].minimum, valpp[i].maximum));
> }
> }
> return (EOPNOTSUPP);
> Index: kern/kern_tc.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_tc.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 kern_tc.c
> --- kern/kern_tc.c 23 Feb 2021 04:44:31 -0000 1.71
> +++ kern/kern_tc.c 28 Apr 2021 10:04:02 -0000
> @@ -829,7 +829,7 @@ inittimecounter(void)
> }
>
> const struct sysctl_bounded_args tc_vars[] = {
> - { KERN_TIMECOUNTER_TICK, &tc_tick, 1, 0 },
> + { KERN_TIMECOUNTER_TICK, &tc_tick, SYSCTL_INT_READONLY },
> { KERN_TIMECOUNTER_TIMESTEPWARNINGS, ×tepwarnings, 0, 1 },
> };
>
> Index: kern/sysv_sem.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/sysv_sem.c,v
> retrieving revision 1.60
> diff -u -p -r1.60 sysv_sem.c
> --- kern/sysv_sem.c 17 Nov 2020 03:23:54 -0000 1.60
> +++ kern/sysv_sem.c 28 Apr 2021 10:04:02 -0000
> @@ -860,10 +860,10 @@ sema_reallocate(int val)
> }
>
> const struct sysctl_bounded_args sysvsem_vars[] = {
> - { KERN_SEMINFO_SEMUME, &seminfo.semume, 1, 0 },
> - { KERN_SEMINFO_SEMUSZ, &seminfo.semusz, 1, 0 },
> - { KERN_SEMINFO_SEMVMX, &seminfo.semvmx, 1, 0 },
> - { KERN_SEMINFO_SEMAEM, &seminfo.semaem, 1, 0 },
> + { KERN_SEMINFO_SEMUME, &seminfo.semume, SYSCTL_INT_READONLY },
> + { KERN_SEMINFO_SEMUSZ, &seminfo.semusz, SYSCTL_INT_READONLY },
> + { KERN_SEMINFO_SEMVMX, &seminfo.semvmx, SYSCTL_INT_READONLY },
> + { KERN_SEMINFO_SEMAEM, &seminfo.semaem, SYSCTL_INT_READONLY },
> { KERN_SEMINFO_SEMOPM, &seminfo.semopm, 1, INT_MAX },
> };
>
> Index: netinet/ip_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.358
> diff -u -p -r1.358 ip_input.c
> --- netinet/ip_input.c 23 Apr 2021 21:55:36 -0000 1.358
> +++ netinet/ip_input.c 28 Apr 2021 10:04:02 -0000
> @@ -113,7 +113,7 @@ extern int ip_mrtproto;
>
> const struct sysctl_bounded_args ipctl_vars[] = {
> #ifdef MROUTING
> - { IPCTL_MRTPROTO, &ip_mrtproto, 1, 0 },
> + { IPCTL_MRTPROTO, &ip_mrtproto, SYSCTL_INT_READONLY },
> #endif
> { IPCTL_FORWARDING, &ipforwarding, 0, 2 },
> { IPCTL_SENDREDIRECTS, &ipsendredirects, 0, 1 },
> Index: netinet/tcp_usrreq.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_usrreq.c,v
> retrieving revision 1.180
> diff -u -p -r1.180 tcp_usrreq.c
> --- netinet/tcp_usrreq.c 10 Mar 2021 10:21:49 -0000 1.180
> +++ netinet/tcp_usrreq.c 28 Apr 2021 10:04:02 -0000
> @@ -112,7 +112,7 @@ u_int tcp_autorcvbuf_inc = 16 * 1024;
>
> static int pr_slowhz = PR_SLOWHZ;
> const struct sysctl_bounded_args tcpctl_vars[] = {
> - { TCPCTL_SLOWHZ, &pr_slowhz, 1, 0 },
> + { TCPCTL_SLOWHZ, &pr_slowhz, SYSCTL_INT_READONLY },
> { TCPCTL_RFC1323, &tcp_do_rfc1323, 0, 1 },
> { TCPCTL_KEEPINITTIME, &tcptv_keep_init, 1, 3 * TCPTV_KEEP_INIT },
> { TCPCTL_KEEPIDLE, &tcp_keepidle, 1, 5 * TCPTV_KEEP_IDLE },
> Index: netinet6/ip6_input.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
> retrieving revision 1.232
> diff -u -p -r1.232 ip6_input.c
> --- netinet6/ip6_input.c 10 Mar 2021 10:21:49 -0000 1.232
> +++ netinet6/ip6_input.c 28 Apr 2021 10:04:02 -0000
> @@ -1339,9 +1339,9 @@ extern int ip6_mrtproto;
> #endif
>
> const struct sysctl_bounded_args ipv6ctl_vars[] = {
> - { IPV6CTL_DAD_PENDING, &ip6_dad_pending, 1, 0 },
> + { IPV6CTL_DAD_PENDING, &ip6_dad_pending, SYSCTL_INT_READONLY },
> #ifdef MROUTING
> - { IPV6CTL_MRTPROTO, &ip6_mrtproto, 1, 0 },
> + { IPV6CTL_MRTPROTO, &ip6_mrtproto, SYSCTL_INT_READONLY },
> #endif
> { IPV6CTL_FORWARDING, &ip6_forwarding, 0, 1 },
> { IPV6CTL_SENDREDIRECTS, &ip6_sendredirects, 0, 1 },
> Index: sys/sysctl.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/sys/sysctl.h,v
> retrieving revision 1.214
> diff -u -p -r1.214 sysctl.h
> --- sys/sysctl.h 10 Mar 2021 10:21:47 -0000 1.214
> +++ sys/sysctl.h 28 Apr 2021 10:04:02 -0000
> @@ -1000,6 +1000,9 @@ struct sysctl_bounded_args {
> int maximum; /* read-only variable if minimum > maximum */
> };
>
> +#define SYSCTL_INT_UNBOUNDED 0,0
Unused, maybe introduce a single usage as an example?
> +#define SYSCTL_INT_READONLY 1,0
> +
> /*
> * Internal sysctl function calling convention:
> *
