Create a new syscall to avoid two calls to __sysctl: first to resolve the name, and second to actually execute the action. This call pattern is quite common so call kernel just once.
Tested with older and newer binaries on amd64 and compat32.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| lib/libc/gen/sysctlbyname.c | ||
|---|---|---|
| 55–73 | this should be namelen to avoid namespace collision with newlen the strlen call should be done in a separate line | |
| sys/compat/freebsd32/freebsd32_misc.c | ||
| 2269 | namelen is unsigned, so this should just check for == 0 instead of < 1 | |
| 2272 | vast majority of names are short, afair < 16. this should have a buffer of similar size to avoid having to do malloc in the common case. | |
| 2278 | why the magic numbers | |
| 2293 | the entire point of a __sysctlbyname syscall is to do all the sysctl work *once*. that is, the code should be reorganized so that the handler can be looked up by the name and then executed. this patch, just like the current approach, first finds the oid number and then performs yet another lookup to translate it the handler. this basically finds the handler twice. it does provide an upshot of only one syscall trip, but I don't think that's good enough of an improvement to integrate. | |
| 2298 | this ignored the error | |
| sys/compat/freebsd32/freebsd32_misc.c | ||
|---|---|---|
| 2293 | OK, i'll rewrite this. | |
| 2298 | So did every other usage of this function in this file, and in most cases in the whole kernel. I don't know if assigning EFAULT now, when the action was potentially made if this was write/set and not read only sysctl makes any sense here. | |
| lib/libc/gen/sysctlbyname.c | ||
|---|---|---|
| 50–53 | What is the point of creating this single-use variable ? Esp. of type u_int which theoretically causes truncation ? | |
| lib/libc/sys/Symbol.map | ||
| 402 | This is the wrong version. | |
| 469 | And I do not understand this line at all. | |
| sys/compat/freebsd32/freebsd32_misc.c | ||
| 2286 | if (error != 0) | |
| 2289 | if (oldlenp != NULL) | |
| sys/compat/freebsd32/freebsd32_proto.h | ||
| 713 ↗ | (On Diff #48428) | Remove generated parts from the review. |
| sys/compat/freebsd32/syscalls.master | ||
| 1118 ↗ | (On Diff #48428) | Why namelen is u_int while newlen uint32_t ? |
Comment Actions
I think the kernel part is operational well enough. With this cleaned up I think it may be fine for inclusion - after we get userspace to use the syscall we can improve internals much later.
Do you still have interest in working on this?
Comment Actions
It would be helpful to generate this diff against HEAD. That would exclude the generated files and the freebsd32 capabilities.conf entry.
| sys/compat/freebsd32/syscalls.master | ||
|---|---|---|
| 1118 ↗ | (On Diff #48428) | both should be size_t. The implementation should be freebsd32___sysctlbyname(). |
| sys/kern/syscalls.master | ||
| 2856 | Please don't add more instances of this last line. Just live with the extra underscores in the code. | |
Comment Actions
Kernel code has to get deduplicated. You can pass relevant sizes down and just have one variant do the trick.
Drop the NAMEBUFLEN macro. You can just sizeof(buf) instead for comparison.
This needs a TODO comment explaining the lookup should be done once.
Apart from this I have no further comments about kernel part.
One may consdier a temporary fallbackin libc in case of ENOSYS where the code redoes work the old way. Should save some headache during upgrades. To be removed in few months time.
Comment Actions
Split the function for one that do the actual work and teach sys___sysctlbyname and freebsd32___sysctlbyname to call it properly.
Provide a compat layer in libc by ignoring the SIGSYS that can be generated by the kernel when the new libc is used with older kernel. As calling sigaction(2) twice every time is rather expensive and possibly intrusive I'd suggest to keep it around for a rather short period of time, say a few weeks and announce the change on current@ to remind folks to reboot with the new kernel before installworld. I provided an #ifndef HAVE___SYSCTLBYNAME for someone who wants to force the build without libc compat shims, but I'm not entirely happy with this solution either.
| lib/libc/gen/sysctlbyname.c | ||
|---|---|---|
| 60 | This is no-go outright. Library must not manipulate global process state. It is both non-thread-safe and non-async-signal-safe. If you want to handle older kernels in libc(why ?) use __FreeBSD_version. Look for examples. | |
| lib/libc/gen/sysctlbyname.c | ||
|---|---|---|
| 60 | I agree, it was just an idea to ease the upgrade process for those installing new libc while running older kernel, because even reboot would be killed by SIGSYS as it use sysctlbyname(3). | |
| lib/libc/gen/sysctlbyname.c | ||
|---|---|---|
| 60 | My opinion is that newer libc requires newer kernel, but some people are nicer to users. Look at lib/libc/sys/stat.c for what I mean by checks for __FreeBSD_version. | |
Comment Actions
Thanks for doing the compat shim. I think this would have been one area where it would have been hard to recover from a make installworld before a new kernel was in place...
Comment Actions
Kernel part looks legit to me modulo some nits which I'm not going to bother with.
If others accept the userspace part I'll commit this later.