Permit the kernel environment to set an array of numeric values for a single sysctl(9) node.

[• hselasky]

Fix whitespace and long line.

[imp]

I think this is good. I'm a little hesitant about the suffix part of it, but know of no good way to flag it's acceptance (I know of several ways, they are all complex and error-prone though).
so, I'd feel better with someone else also blessed the change.

[kib]

kern/kern_environment.c

548 ↗

(On Diff #43750)

I am confused by this loop control. You start with a random garbage on stack for the initial buf[] content.

[• hselasky]

kern/kern_environment.c

548 ↗

(On Diff #43750)

ptr = buf, means address of buf. buf is filled with a valid string after the getenv_string() call.

[kib]

kern/kern_environment.c

545 ↗

(On Diff #43750)

The comment is useless.

548 ↗

(On Diff #43750)

Ah, I missed it.

552 ↗

(On Diff #43750)

Style: declare vars at the top of the function.

554 ↗

(On Diff #43750)

Well, almost all of the comments trivially rephrase the code.

587 ↗

(On Diff #43750)

This accepts arbitrary garbage after the numeric part.

614 ↗

(On Diff #43750)

Now I am confused by this comparision. Valid range for unsigned char on all supported arches is 0..255.

619 ↗

(On Diff #43750)

Same note for uint16_t.

624 ↗

(On Diff #43750)

And there.

[• hselasky]

kern/kern_environment.c

614 ↗

(On Diff #43750)

I'll fix.

[• hselasky]

Address comments from kib@

[jhb]

sys/kern/kern_environment.c

539 ↗

(On Diff #43768)

I think it would be simpler to just add a 'bool signed' parameter instead of overloading type_size. type_size can then always be 'sizeof(foo)' which would be easier to read at the call-site. If you'd rather not use a bool, then define a simple enum:

enum sign {
    SIGNED,
    UNSIGNED,
};

And use that instead so you have something like this for CTLTYPE_UINT:

error = getenv_array(path + rem, data, sizeof(data), &size, sizeof(int), UNSIGNED);

sys/kern/kern_sysctl.c

195 ↗

(On Diff #43768)

Using a uint64_t array initially threw me off. I wonder if it wouldn't be more accurate to declare this as 'char data[1024] __Alignas(uint64_t)' or the like?

[• hselasky]

Update patch as per jhb@'s suggestions.

[kib]

sys/sys/systm.h

349 ↗

(On Diff #43819)

0 should be false, since you use bool as the function argument type.

350 ↗

(On Diff #43819)

1 -> true

351 ↗

(On Diff #43819)

Wrap the line.

[• hselasky]

Update as per kib@'s suggestions.

[• hselasky]

jhb@ ping.

[jhb]

sys/sys/systm.h

349 ↗

(On Diff #43820)

I would perhaps move these constants up out of the list of function prototypes, however the prevailing style in this file seems to be to include them without a comment below a function prototype followed by a blank line before the next function prototype (see HD_* after hexdump(), HASH_* after hashinit_flags()).