In article <21271.19186.176517.832...@guava.gson.org>, Andreas Gustafsson <g...@netbsd.org> wrote: >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_syctl.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?
I think you are correct. christos