Page MenuHomeFreeBSD

sysctlmibinfo(3)
Needs ReviewPublic

Authored by alfix86_gmail.com on Dec 31 2018, 12:10 AM.

Details

Reviewers
None
Group Reviewers
manpages
Summary

The sysctlmibinfo library is an interface to the kernel sysctl-mib-tree.
It implements wrappers around undocumented "sysctl.*" kernel states to get mib-entries information and provides a convenient API to build a mib-entry, entries-list and mib-tree in userspace.

The advantages to have sysctlmibinfo are:

  • an easy API to the kernel sysctl mib-tree,
  • a handy mib-entry implementation,
  • building entries-list and mib-tree in userspace,
  • writing quickly a custom sysctl(8) tool,
  • changes to kern_sysctl.c interface won't upset userspace tools.
Test Plan

Files *.c in examples/ show every function

Tools using libsysctl

  • "sysctlview 0.1", a sysctl gtk tool
  • "nsysctl 0.1", sysctl(8) + [-FIlMmSy] + [--libxo]

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

bcr added a subscriber: bcr.Dec 31 2018, 11:32 AM

Thanks for your work! I found one issue with the man page.
Can you run "mandoc -Tlint" and the textproc/igor package over the man page and see if additional things are reported?

lib/libsysctl/libsysctl.3
113 ↗(On Diff #52443)

There must be a line break after a sentence stop.

lib/libsysctl/libsysctl.3
113 ↗(On Diff #52443)

Thank you for your suggestion, I'm updating to fix "mandoc -Tlint" and "igor -Dd"

brooks added a subscriber: brooks.Jan 4 2019, 6:07 PM

This looks like a neat library. I've made a few comments based on a casual review.

lib/libsysctl/libsysctl.3
293 ↗(On Diff #52443)

I think I'd just talk about allocated memory rather than heap.

lib/libsysctl/libsysctl.c
90 ↗(On Diff #52443)

Casting to void* before passing as a void* argument seems unnecessary.

171 ↗(On Diff #52443)

This feels like it should be done in a temporary with *idnextlen only updated on success (or perhaps unconditionally zeroed).

226 ↗(On Diff #52443)

No need to cast malloc().

241 ↗(On Diff #52443)

These are redundant to the bzero.

243 ↗(On Diff #52443)

Using memset() would allow the compiler to elide much of the zeroing.

255 ↗(On Diff #52443)

This leaks memory as do all the other malloc failure paths in this function.

315 ↗(On Diff #52443)

looks like too much indentation for style(9) (but could be phabricator).

448 ↗(On Diff #52443)

Declaration should be at the top of the function.

lib/libsysctl/libsysctl.h
44 ↗(On Diff #52443)

I'd find the headers a lot easier to read if you included variable names in the function prototypes.

79 ↗(On Diff #52443)

While this layout (pairing length and pointer) makes the structure easy to read, it's somewhat expensive on architectures where pointers are larger and have stronger alignment constraints than size_t. E.g. on 128-bit CHERI this layout would waste an extra 32-bytes on padding which could add up if you have a lot of these.

90 ↗(On Diff #52443)

I'd say children here.

cem added a subscriber: cem.Jan 4 2019, 8:30 PM
bapt added a subscriber: bapt.Jan 7 2019, 12:19 PM

why not adding that to libutils instead of adding another library?

This comment was removed by alfix86_gmail.com.

Thanks you all for the comments,
I updated to fix, soon the "update diff".

In D18696#400437, @bapt wrote:

why not adding that to libutils instead of adding another library?

I used "libsysctl_" prefix to avoid naming conflict from <sys/sysctl.h>. Moreover a programmer easily distinguishes kernel space and userspace: sysctl_oid / libsysctl_object, CTL_MAXNAME / LIBSYSCTL_IDMAXLEVEL, etc. Anyway can the prefix "libsysctl" be kept in libutil? Should I use a new prefix "sysctl_util_" or "util_sysctl_" for libutil? What do you prefer?

lib/libsysctl/libsysctl.3
293 ↗(On Diff #52443)

fixed, change: "heap" -> "allocated memory"

lib/libsysctl/libsysctl.c
90 ↗(On Diff #52443)

fixed, delete: Casting to void* before passing as a void* argument

171 ↗(On Diff #52443)

fixed: done in a temporary, *idnextlen only updated on success.

226 ↗(On Diff #52443)

fixed, delete: cast malloc()

241 ↗(On Diff #52443)

fixed, delete: redundant to the bzero.

243 ↗(On Diff #52443)

fixed, change: bzero() -> memset()

255 ↗(On Diff #52443)

fixed, add: libsysctl_freeobject(obj) after each malloc failure

315 ↗(On Diff #52443)

fixed: used 'uncrustify' to check style(9)

448 ↗(On Diff #52443)

fixed: Declarations are at the top of the functions.

lib/libsysctl/libsysctl.h
44 ↗(On Diff #52443)

fixed: added variable names in the function prototypes.

79 ↗(On Diff #52443)

(Thanks you, very important suggestion).
Fixed, delete: namelen, desclen, labellen, fmtlen.
remain: idlen, id isn' t a static array then a can't use nitems() to get back the level of the entry.

90 ↗(On Diff #52443)

Fixed, change: "childs" -> "children"
(I left the UK 3 years ago, it' s time to refresh my English).

bapt added a comment.Jan 10 2019, 2:03 PM
In D18696#400437, @bapt wrote:

why not adding that to libutils instead of adding another library?

I used "libsysctl_" prefix to avoid naming conflict from <sys/sysctl.h>. Moreover a programmer easily distinguishes kernel space and userspace: sysctl_oid / libsysctl_object, CTL_MAXNAME / LIBSYSCTL_IDMAXLEVEL, etc. Anyway can the prefix "libsysctl" be kept in libutil? Should I use a new prefix "sysctl_util_" or "util_sysctl_" for libutil? What do you prefer?

I do not see why you could not, libutil is not a consistent library in term of API it is more a collection of "hey that is useful but we don't want to bloat our libc".

For example looks at pw_util(3) it is prefixed by pw_
I have no strong opinion about the prefix you should use, libsysctl sounds ok to me, don't know what others thinks

Complete manual, checked by "mandoc -Tlint" and "igor -Dd".

comments and suggestions applied,
(particularly: avoid memory leak, delete some struct member for alignment constraints and fixed style(9) ).

dab added a subscriber: dab.Jan 10 2019, 7:51 PM
dab added inline comments.
lib/libsysctl/libsysctl.3
343–349 ↗(On Diff #52741)

Again, here it both says the library isn't designed to get/set kernel state, yet the macros are defined anyway. See comments above about this.

If these are kept, I would rephrase the text here:

[...]
.Xr sysctl 3 .
However, some macros are defined for this purpose:
[...]

lib/libsysctl/libsysctl.h
152 ↗(On Diff #52741)

These are very thin wrappers around sysctl() and I don't think provide significant value. Why define them?

At first I thought maybe the GET was going to address the common case where the value length is not known and you have to round-trip twice, first to get the length and then a second time to get the actual value, but that isn't the case. I think that would provide value.

I would just delete these as they are currently defined.

158 ↗(On Diff #52741)

nit: s/macro is/macros are/

lib/libsysctl/libsysctl.3
343–349 ↗(On Diff #52741)

answer up

lib/libsysctl/libsysctl.h
152 ↗(On Diff #52741)

Thank you for your comment,
OK I 'll delete _GETVALUE(), _SETVALUE() and update the manual.

first to get the length and then a second time to get the actual value

I think you mean: LIBSYSCTL_VALUELEN(id, idlevel, size) sysctl(id, idlevel, NULL, size, NULL, 0) , if you wish I could add it, anyway it's right: these are very thin wrappers.

158 ↗(On Diff #52741)

answer up

alfix86_gmail.com edited the summary of this revision. (Show Details)
alfix86_gmail.com edited the test plan for this revision. (Show Details)

libsysctl moved to libutil,
thin wrappers deleted ( GETVALUE() and SETVALUE() ),
manual updated and
fixed style(9).

danfe added a subscriber: danfe.Jan 14 2019, 2:16 AM

I used "libsysctl_" prefix to avoid naming conflict from <sys/sysctl.h>. Moreover a programmer easily distinguishes kernel space and userspace: sysctl_oid / libsysctl_object, CTL_MAXNAME / LIBSYSCTL_IDMAXLEVEL, etc.

I understand the motivation, that's a fair point.

Anyway can the prefix "libsysctl" be kept in libutil? Should I use a new prefix "sysctl_util_" or "util_sysctl_" for libutil? What do you prefer?

Personally I find lib prefix redundant for library functions in general, yet sysctl_util_ looks a bit too verbose. You might want to take a look at /usr/include/capsicum_helpers.h and their caph prefix (sysctlh or perhaps sysctl_tree since these functions mainly revolve around sysctl trees).

Style(9)-wise, I think you could trim excessive double newlines (between the functions and sometimes even within a function).

Thank you for your comment,

I used "libsysctl_" prefix to avoid naming conflict from <sys/sysctl.h>. Moreover a programmer easily distinguishes kernel space and userspace: sysctl_oid / libsysctl_object, CTL_MAXNAME / LIBSYSCTL_IDMAXLEVEL, etc.

Personally I find lib prefix redundant for library functions in general, yet sysctl_util_ looks a bit too verbose. You might want to take a look at /usr/include/capsicum_helpers.h and their caph prefix (sysctlh or perhaps sysctl_tree since these functions mainly revolve around sysctl trees).

Then I could use sysctlmif_ and rename files to sysctlmibinfo.h/c/3 .

Style(9)-wise, I think you could trim excessive double newlines

I' ll update to fix.

danfe added a comment.Jan 18 2019, 6:36 AM

Then I could use sysctlmif_ and rename files to sysctlmibinfo.h/c/3 .

That looks concise yet specific enough and makes sense, I think like it. :-)

alfix86_gmail.com retitled this revision from Manage sysctl-tree in userpace to sysctlmibinfo(3).
alfix86_gmail.com edited the summary of this revision. (Show Details)
alfix86_gmail.com edited the test plan for this revision. (Show Details)
  • Change prefix: libsysctl_ -> sysctlmif_
  • manual page: update API
  • rename files: libsysctl.h/c/3 -> sysctlmibinfo.h/c/3
  • update lib/libutil/Makefile
  • fix style(9): delete 'double newlines'

Found two things which may need some TLC.

lib/libutil/sysctlmibinfo.3
145 ↗(On Diff #52989)

Maybe you should rephrase this as "as it is not designed to get and set entry values, anyone wishing to do this should see sysctl(3)"?

210 ↗(On Diff #52989)

Doesn't this sentence become a double negative?

dab added inline comments.Jan 28 2019, 1:16 PM
lib/libutil/sysctlmibinfo.3
210 ↗(On Diff #52989)

s/has not a/has the/

  • add: __FBSDID("$FreeBSD$").
  • indent: sysctlmif_internal_subtree().
  • change: SYSCTLMIF_IDMAXLEVEL -> SYSCTLMIF_MAXIDLEVEL ( = CTL_MAXNAME).
  • change: mib-size for magic nodes to mib[CTL_MAXNAME + 2] (like in sbin/sysctl.c).
  • add free(object->fmt) in sysctlmif_freeobject() to avoid memory leak.
  • manual page: fix comments and update API.
alfix86_gmail.com added a comment.EditedJan 29 2019, 1:00 AM

Thank you for comments, I updated the manual page

lib/libutil/sysctlmibinfo.3
145 ↗(On Diff #52989)

Ok, I'll update

210 ↗(On Diff #52989)

Thank you, I' ll change to fix

Question: why "sysctlmif", as opposed to "sysctlmib"? Since this is about walking the MIB tree?

Question: why "sysctlmif", as opposed to "sysctlmib"? Since this is about walking the MIB tree?

Thank you for the comment,

"sysctlmif" - "sysctlmibinfo", <sysctlmibinfo.h>, man sysctlmibinfo, sysctlmif_*(), etc. , describes the purpose of this API: getting entry-info (name, desc, next, ...) and possibly building some list or (sub) tree of "mif_objects". Before I thought to use "sysctlmib" to rename "libsysctl" , but it seemed to me that it would be suitable for a "complete" mib API.

Anyway I have no strong opinion about the prefix/naming, What do you prefer? sysctlmib.h/.c/.3/_* ?

Anyway I have no strong opinion about the prefix/naming, What do you prefer? sysctlmib.h/.c/.3/_* ?

No changes, I was just curious. :-) Your explanation makes sense. Thanks!