Re: CVS commit: src/sys/kern

[Andreas Gustafsson <gson%NetBSD.org@localhost>]

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?
-- 
Andreas Gustafsson, gson%NetBSD.org@localhost

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
 }