Page MenuHomeFreeBSD
Differential D18696

sysctlmibinfo(3)
AbandonedPublic
Actions

Authored by asiciliano on Dec 31 2018, 12:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 26, 3:56 PM
Unknown Object (File)
Wed, Apr 24, 5:52 PM
Unknown Object (File)
Wed, Apr 24, 12:21 PM
Unknown Object (File)
Sat, Apr 20, 6:31 AM
Unknown Object (File)
Thu, Apr 18, 3:17 AM
Unknown Object (File)
Feb 29 2024, 6:24 AM
Unknown Object (File)
Feb 28 2024, 11:18 PM
Unknown Object (File)
Feb 22 2024, 12:45 PM
View All 33 Files
Subscribers
bapt
bcr
brooks
cem
dab
danfe
debdrup
View All 10 Subscribers

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

asiciliano created this revision.Dec 31 2018, 12:10 AM
Herald added a reviewer: manpages. · View Herald TranscriptDec 31 2018, 12:10 AM
Herald added subscribers: Contributor Reviews (src), imp. · View Herald Transcript
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.

asiciliano added inline comments.Jan 1 2019, 11:53 AM
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?

asiciliano added a comment.Jan 7 2019, 5:21 PM
This comment was removed by asiciliano.
asiciliano added a comment.Jan 10 2019, 1:34 PM

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

asiciliano updated this revision to Diff 52741.Jan 10 2019, 3:44 PM

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/

asiciliano added inline comments.Jan 10 2019, 9:13 PM
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

asiciliano updated this revision to Diff 52769.Jan 11 2019, 2:55 PM
asiciliano edited the summary of this revision. (Show Details)
asiciliano 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
In D18696#401385, @alfix86_gmail.com wrote:

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).

asiciliano added a comment.Jan 17 2019, 7:21 PM

Thank you for your comment,

In D18696#402107, @danfe wrote:
In D18696#401385, @alfix86_gmail.com wrote:

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
In D18696#402995, @alfix86_gmail.com wrote:

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. :-)

asiciliano updated this revision to Diff 52989.Jan 18 2019, 4:18 PM
asiciliano retitled this revision from Manage sysctl-tree in userpace to sysctlmibinfo(3).
asiciliano edited the summary of this revision. (Show Details)
asiciliano 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'
debdrup added a subscriber: debdrup.Jan 27 2019, 7:58 PM

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/

asiciliano updated this revision to Diff 53353.Jan 29 2019, 12:58 AM
  • 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.
asiciliano 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

rpokala added a subscriber: rpokala.Jan 29 2019, 2:05 AM

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

asiciliano added a comment.Jan 30 2019, 8:08 PM
In D18696#406259, @rpokala wrote:

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/_* ?

rpokala added a comment.Jan 30 2019, 8:32 PM

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!

asiciliano abandoned this revision.Feb 3 2021, 1:25 AM

Abandon Revision: This library is obsoleted by <devel/libsysctlmibinfo2>,
Thank you for the suggestions and reviews.

Revision Contents
Changeset List

PathSize
lib/
libutil/
6 lines
151 lines
418 lines
541 lines

Diff 52769

View Options

lib/libutil/Makefile

Loading...
View Options

lib/libutil/libsysctl.h

Loading...
View Options

lib/libutil/libsysctl.3

Loading...
View Options

lib/libutil/libsysctl.c

Loading...