Date: Wed, 17 Oct 2007 20:38:10 -0400 From: "Constantine A. Murenin" <cnst@FreeBSD.org> To: John Baldwin <jhb@FreeBSD.org> Cc: src-committers@FreeBSD.org, Alexander Leidinger <netchild@FreeBSD.org>, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org, "Constantine A. Murenin" <cnst@FreeBSD.org>, Poul-Henning Kamp <phk@phk.freebsd.dk>, Wilko Bulte <wb@freebie.xs4all.nl> Subject: Re: 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 Message-ID: <4716AAF2.5040209@FreeBSD.org> In-Reply-To: <200710170903.31724.jhb@freebsd.org> References: <f34ca13c0710161446l50deee86k3e6d6605e35a79d6@mail.gmail.com> <200710161809.10755.jhb@freebsd.org> <47153F62.1080709@FreeBSD.org> <200710170903.31724.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 17/10/2007 09:03, John Baldwin wrote: > 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@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. I guess I see where you're coming from here. You are concerned that the hack to register sensor_attach'ed sensors with the sysctl(8) magic all at once may conflict with additional drivers, for example, if sensor_attach is called after sensordev_install. OpenBSD 4.2 has more than 50 drivers using this framework, and all of them satisfy the assertion that the above doesn't happen -- no sensor_attach calls are ever made after sensordev_install. On a similar note, the sensor_detach function is redundant -- the few drivers that actually use it won't notice any difference if it is removed in its entirety -- sensordev_deinstall automatically removes all sensors of a given sensordev. (Granted, this might be documented a bit more explicitly, but this has not been a problem so far.) In other words, it is usually the devices that hold sensors that are hotpluggable, not the individual sensors themselves. So in the realm of the sensor framework, the sysctl magic for sysctl(8) is not considered to be a temporary hack. >>>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. You are missing some point here. With this interface, sysctl is simply used as a transfer mechanism to export two structures: * struct sensordev, which defines how further sensors on a given device can be addressed and how many sensors of each type a given device has (this is something that would not be possible to export via the sysctl magic alone, BTW) * struct sensor, which provides readings of individual sensors on a given sensor device The above gives this framework the potential to be source-code compatible with any kind of sysctl interface, be that original non-dynamic sysctl, FreeBSD's dynamic sysctl, or possibly even NetBSD's dynamic sysctl. It also provides additional benefits -- the whole tree of a given sensordev can be traversed with completely ignoring sensors of certain types. What you are suggesting is that a FreeBSD-only sysctl-specific functions are used instead, also limiting the additional functionality provided by sensordev structure, but I see no technical benefit in this. (I hope my explanation above about the specific hack in relation to OpenBSD's existing drivers addresses your concern for good.) Thus the two-layered version of the framework (sensordev/sensor) defines its own namespace. Granted, on OpenBSD this is much more valuable than it is on FreeBSD, since FreeBSD already has dynamic sysctl mechanism (which is quite polluted by irrelevant and unused values, IMHO). However, many people are quite certain that the sensor framework is nonetheless valuable even on FreeBSD, too. It provides something that the plain FreeBSD dynamic sysctl interface doesn't provide -- simplicity of use for sensor-like devices and a common namespace with clearly defined restrictions. Cheers, Constantine.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4716AAF2.5040209>