Re: CVS commit: src/sys/kern

[christos%astron.com@localhost (Christos Zoulas)]

In article <21271.19186.176517.832253%guava.gson.org@localhost>,
Andreas Gustafsson  <gson%NetBSD.org@localhost> 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