cvs commit: src/etc Makefile sensorsd.conf src/etc/defaults rc.conf src/etc/rc.d Makefile sensorsd src/lib/libc/gen sysctl.3 src/sbin/sysctl sysctl.8 sysctl.c src/share/man/man5 rc.conf.5 src/share/man/man9 Makefile sensor_attach.9 src/sys/conf f

[John Baldwin]

On Tuesday 16 October 2007 06:46:58 pm Constantine A. Murenin wrote:
> On 16/10/2007 18:09, John Baldwin wrote:
> 
> > On Tuesday 16 October 2007 05:46:18 pm Constantine A. Murenin wrote:
> > 
> >>On 16/10/2007, John Baldwin <jhb at freebsd.org> wrote:
> >>
> >>>On Tuesday 16 October 2007 12:33:11 pm Alexander Leidinger wrote:
> >>>
> >>>>Constantine asked for review several times on -current. He got some
> >>>>reviews several times for commits to perforce. He incorporated
> >>>>suggestions from those reviews, or explained why it is like it is and
> >>>>why he can not switch (with no replies with suggestions how to solve
> >>>>the problems he sees with the suggestions). Now you come and ask why
> >>>>nobody pointed out some flaws before (without telling us which
> >>>>technical flaws you talk about).
> >>>
> >>>At least from my point of view this is not quite accurate as pretty much 
> > 
> > all
> > 
> >>>my feedback to the p4 commits was ignored with basically "Well, I don't 
> > 
> > like
> > 
> >>>doing it that way".  Specifically, with regards to creating dynamic sysctl
> >>>trees, Constantine feels that sysctl_add_oid(9) is a hack rather than
> >>>recognizing that this is a feature of FreeBSD's sysctl system despite
> >>>repeated e-mails on the subject.
> >>
> >>Dear John,
> >>
> >>I have specifically addressed this concern of yours just several weeks ago.
> >>
> >>Please see the following message, which you've never replied to:
> >>
> >>http://lists.freebsd.org/pipermail/p4-projects/2007-September/021121.html
> >>
> >>I've used the documented parts of the FreeBSD's sysctl interface to
> >>preserve 100% userland compatibility with OpenBSD.
> > 
> > 
> > FreeBSD already provides an interface for creating dynamic sysctl trees that 
> > is simpler than having to simulate a pseudo-tree via the .oid_handler 
> > routine.  In some cases (such as kern.proc) FreeBSD still uses a function 
> > handler rather than giving each process its own sysctl node.  However, in 
> > other cases (generally more recent ones, and ones not as large/performance 
> > impacting) such as dev.* or the recent proposal to give ifnet's their own 
> > nodes under 'net.if' or the like, sysctl_add_oid(9) is used.
> 
> Which one is simpler is questionable.  Take one more look at the number 
> of different macros that are available in the sysctl(9) and friends, and 
> then compare with four functions of the sensors framework:
> 
> * sensor_attach(9) to attach a sensor to a sensordev tree
> * sensordev_install(9) to register the sensordev tree with sysctl(9)
> and the same with the "de" prefix
> 
> sensordev_install(9) acts similarly to the sysctl(9) ctx concept, but is 
> geared towards the sensors approach
> 
> If you want to make sure you don't attract any new contributors, then 
> certainly the bunch of the sysctl(9) macros are simpler.  ;)

I never said to not have sensor_*() routines.  I just think that the sensor
implementation should make use of dynamic sysctls to build the sysctl
interface rather than treating dynamic sysctls as a temporary hack and adding
a duplicate interface for walking the hw.sensors tree.
 
> > As to the process of walking sysctl trees being undocumented, it is simply 
> > missing a wrapper routine ala sysctlbyname(3) and a manpage.  You could 
> > easily provide one and thus provide a facility for enumerating many different 
> > things than having several one-off oid_handler routines to enumerate 
> > subtrees.  However, it is not some "backdoor" hack interface anymore than 
> > sysctlbyname(3) is.  They are both equally hackish or non-hackish (depending 
> > on your point of view).
> 
> This interface is about having a special tree for the hardware sensors, 
> with special rules regarding the namespace.  What you suggest is that I 
> try to abandon this, but then why do you need this framework to start with?

I suggest you think in layered abstractions.  sysctl is a tool for creating
a namespace that the kernel exports to userland.  Let sysctl manage the
namespace and the sensor code just populate nodes in the namespace and manage
sensors.

I did some thinking last night about what I would really want out of a sensors
framework btw, and will send a mail to arch@ about that.  The short version
though is that I do think the existing sensors code has a lot of things right
(The sensor structure seems very reasonable for example.  I might have done 4
states rather than 3, but 3 is quite workable as is.)  The major hangups I
think are requiring sensors to be a kernel user interface rather than a
userland interface and the idea that that is somehow more secure, but more on
that in my e-mail to arch at .

-- 
John Baldwin