kern/param: implement generalized enum sysctl reporting function

[ehem_freebsd_m5p.com]

Summary

This allows having a common function for reporting multiple enums with
the same code. Paramters can be adjusted to suite.

Designated initializers make the CTASSERT() worthless, removed.

[ehem_freebsd_m5p.com]

This is an attempt at a combined function for handling of enum sysctls. Hopefully enums never end up out of range, but I could believe it happening therefore the bounds check.

Biggest issue in my mind is whether I've matched the organization of subr_param.c. Seems reasonable since I'm mostly overwriting the vm_guest sections with equivalents. Now the array of strings is alphabetized since that works with this type of initialization.

[kib]

The helper looks surprisingly not bad. Could there be other uses for some existing sysctls? I am slightly worried that hardcoding e.g. u_int *value makes it less useful than possible.

sys/kern/subr_param.c

344–351

Name this local differently than 'params' and then you can use SYSCTL_HANDLER_ARGS?

I would call it eparams.

348

This blank line is not needed

351

And this

[ehem_freebsd_m5p.com]

In D32132#725171, @kib wrote:

The helper looks surprisingly not bad. Could there be other uses for some existing sysctls? I am slightly worried that hardcoding e.g. u_int *value makes it less useful than possible.

Pretty sure signed/unsigned mismatches are presently standard in the FreeBSD kernel. There would be warnings all over the place if -Wsign-conversion and -Wsign-compare were enabled.

Unsigned was simpler for the test to be correct, even though enum is supposed to be signed. This isn't difficult to remedy if you want such done.

For wider use "last" would be replaced with "first" and "count". Then the range test becomes *params->value - params->first >= params->count. Ideally the names array has the "first" element as 0, unfortunately this means when initializing the first value has to be added/subtracted from every enum, which could be painful if the enum has enough values.

I was pondering the possibility, though I'm unsure how many sysctls are enums behind the scenes.

sys/kern/subr_param.c

344–351

All depends on how far SYSCTL_HANDLER_ARGS is pushed and why. I was thinking it was simply meant as a handy macro and didn't think the arguments got changed that often, but I could believe my guess was wrong.

One issue I see are "arg1" and "arg2" really should be "argptr" and "argnum". The other issue is here the pointer really should be marked as pointing to a constant, similar to SYSINIT() versus C_SYSINIT().

If use of SYSCTL_HANDLER_ARGS is that strongly preferred, I would tend towards "pparams".

[ehem_freebsd_m5p.com]

Adding support for enums not starting at 0. Add small bit to man page.

[ehem_freebsd_m5p.com]

Since the vm_guest enum starts at 0, the example is highly redundant, but is useful as an example of how this can be used. ENXIO seems appropriate for the out of range case, but it should be documented (note: I make no claims of skill with man formatting, hopefully my emulation of the rest of formatting generates something appropriate).

[kib]

I still think that single example is too early to abstract. I can think about e.g. kern.timecounter.hardware and machdep.idle as possible extensions. Differently from kern.vm_guest, they are writeable, and there is no simple list of values. If you can fit that into your framework, perhaps the feature become much more interesting.

sys/kern/subr_param.c

86

This might be nitems(names). Also I am not sure that you need 'first', by using array designated initializer you instantiate all possible indexes anyway, and they you check for NULL.

[ehem_freebsd_m5p.com]

This may be too much abstraction for this early, but notice D31955 is already a second use (though presently using an earlier version). Covering writable enumerated values would be the first likely extension of this, but D31955 is my real goal right now.

sys/kern/subr_param.c

86

nitems(vm_guest_sysctl_params.names) => 0 (downside of a flexible array)

first is for the case of an enum where the least element is negative or large positive number. A negative number would cause misbehavior. A large positive number would consume memory for unused elements.

[ehem_freebsd_m5p.com]

@kib any news on D32132? This seemed like a separate review from D31955, just an issue of what it should look like. D31955 is a distinct issue since something needs to be done to address pvefi.c's reliance on an x86 variable (bootmethod).

[ehem_freebsd_m5p.com]

Updating to match what I've currently got. Pretty well trivial difference, simply keeping things synchronized.

[ehem_freebsd_m5p.com]

@kib any word on D32132? Substantially the same form has been here for a month and no response...

This is a prerequisite for D31955, something like this is needed.

[kib]

In D32132#737522, @ehem_freebsd_m5p.com wrote:

@kib any word on D32132? Substantially the same form has been here for a month and no response...

This is a prerequisite for D31955, something like this is needed.

I do not want to commit infrastructure that only has one use.

For D31955, you clearly need some feedback and agreement from Xen and arm people.

[bcr]

OK from manpages. Please bump the .Dd when committing.