I posted this on source-changes-d earlier today, but Jeff Rizzo asked me to take it to tech-kern.
On Feb 27, David Laight said: > Module Name: src > Committed By: dsl > Date: Thu Feb 27 22:50:52 UTC 2014 > > Modified Files: > src/sys/kern: kern_sysctl.c > > Log Message: > Allow CTLTYPE_INT and CTLTYPE_QUAD to be read and written as either 4 or 8 > byte values regardless of the type. > 64bit writes to 32bit variables must be valid (signed) values. > 32bit reads of large values return -1. > Amongst other things this should fix libm's code that reads machdep.sse > as a 32bit int, but I'd changed it to 64bit (to common up some code). > > > To generate a diff of this commit: > cvs rdiff -u -r1.246 -r1.247 src/sys/kern/kern_sysctl.c In private correspondence with David, I have objected to this commit as well as his earlier change to the type of the machdep.sse sysctl variable. Regrettably, David and I have not been able to reach an agreement, and he has requested that I ask for opinions on a public list. So... 1. I object to the earlier change of the following sysctl variables from type CTLTYPE_INT to type CTLTYPE_QUAD: machdep.fpu_present machdep.osfxsr machdep.sse machdep.sse2 machdep.biosbasemem machdep.biosextmem My understanding of David's rationale for changing them is that it facilitated sharing code with the machdep.xsave_features variable which actually needs to be of type CTLTYPE_QUAD. By my reckoning, the savings from this code sharing amount to eight lines of code. Changing the types of existing sysctl variables breaks both source and binary compatibility and should not be done lightly; that's why we have both both hw.physmem and hw.physmem64, for example. Now, the types of multiple variables have been incompatibly changed for no reason other than saving a few lines of code. I believe the harm caused by the incompatible type change far outweighs the cost of a few added lines of code, and would like the original types to be restored. I am attaching a patch to do so; it also changes the newly added variables machdep.fpu_save and machdep.fpu_save_size to CTLTYPE_INT, but if David really thinks those should be CTLTYPE_QUAD, I will not argue that point. 2. I also object to the change of kern_sysctl.c 1.247. This change attempts to work around the problems caused by the changes to the variable types by making sysctl() return different types depending on the value of the *oldlenp argument. IMO, this is a bad idea. The *oldlenp argument does *not* specify the size of the data type expected by the caller, but rather the size of a buffer. The sysctl() API allows the caller to pass a buffer larger than the variable being read, and conversely, guarantees that passing a buffer that is too small results in ENOMEM. Both of these aspects of the API are now broken: reading a 4-byte CTLTYPE_INT variable now works for any buffer size >= 4 *except* 8, and attempting to read an 8-byte CTLTYPE_QUAD variable into a buffer of less than 8 bytes is now guaranteed to yield ENOMEM *except* if the buffer size happens to be 4. IMO, this behavior violates both the letter of the sysctl() man page and the principle of least astonishment. Also, the work-around is ineffective in the case of a caller that allocates the buffer dynamically using the size given by an initial sysctl() call with oldp = NULL. If the original types of the sysctl variables are restored, this work-around will no longer serve a purpose, and I'm asking for it to be removed. Opinions? -- Andreas Gustafsson, g...@netbsd.org Index: x86_machdep.c =================================================================== RCS file: /cvsroot/src/sys/arch/x86/x86/x86_machdep.c,v retrieving revision 1.63 diff -u -r1.63 x86_machdep.c --- x86_machdep.c 23 Feb 2014 22:38:40 -0000 1.63 +++ x86_machdep.c 3 Mar 2014 18:41:06 -0000 @@ -1056,7 +1056,17 @@ } static void -const_sysctl(struct sysctllog **clog, const char *name, u_quad_t value, int tag) +const_sysctl_int(struct sysctllog **clog, const char *name, int value, int tag) +{ + sysctl_createv(clog, 0, NULL, NULL, + CTLFLAG_PERMANENT | CTLFLAG_IMMEDIATE, + CTLTYPE_INT, name, NULL, + NULL, (u_quad_t)value, NULL, 0, + CTL_MACHDEP, tag, CTL_EOL); +} + +static void +const_sysctl_quad(struct sysctllog **clog, const char *name, u_quad_t value, int tag) { sysctl_createv(clog, 0, NULL, NULL, CTLFLAG_PERMANENT | CTLFLAG_IMMEDIATE, @@ -1115,17 +1125,17 @@ CTL_MACHDEP, CTL_CREATE, CTL_EOL); /* None of these can ever change once the system has booted */ - const_sysctl(clog, "fpu_present", i386_fpu_present, CPU_FPU_PRESENT); - const_sysctl(clog, "osfxsr", i386_use_fxsave, CPU_OSFXSR); - const_sysctl(clog, "sse", i386_has_sse, CPU_SSE); - const_sysctl(clog, "sse2", i386_has_sse2, CPU_SSE2); - - const_sysctl(clog, "fpu_save", x86_fpu_save, CTL_CREATE); - const_sysctl(clog, "fpu_save_size", x86_fpu_save_size, CTL_CREATE); - const_sysctl(clog, "xsave_features", x86_xsave_features, CTL_CREATE); + const_sysctl_int(clog, "fpu_present", i386_fpu_present, CPU_FPU_PRESENT); + const_sysctl_int(clog, "osfxsr", i386_use_fxsave, CPU_OSFXSR); + const_sysctl_int(clog, "sse", i386_has_sse, CPU_SSE); + const_sysctl_int(clog, "sse2", i386_has_sse2, CPU_SSE2); + + const_sysctl_int(clog, "fpu_save", x86_fpu_save, CTL_CREATE); + const_sysctl_int(clog, "fpu_save_size", x86_fpu_save_size, CTL_CREATE); + const_sysctl_quad(clog, "xsave_features", x86_xsave_features, CTL_CREATE); #ifndef XEN - const_sysctl(clog, "biosbasemem", biosbasemem, CPU_BIOSBASEMEM); - const_sysctl(clog, "biosextmem", biosextmem, CPU_BIOSEXTMEM); + const_sysctl_int(clog, "biosbasemem", biosbasemem, CPU_BIOSBASEMEM); + const_sysctl_int(clog, "biosextmem", biosextmem, CPU_BIOSEXTMEM); #endif }