Details

Reviewers

imp
jhb
kevans

Group Reviewers

manpages

Commits

rS365945: Add getenv(9) boolean parsing functions

Summary

These are wrappers around kern_getenv that enable simpler parsing of
boolean values from environment variables. This works for traditional
boolean values like "0" and "1", and also "true" and "false"
(case-insensitive).

Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33511
Build 30780: arc lint + arc unit

Event Timeline

mhorne requested review of this revision.Sep 1 2020, 12:40 PM

mhorne created this revision.

Harbormaster completed remote builds in B33283: Diff 76473.Sep 1 2020, 12:40 PM

mhorne added a reviewer: manpages.Sep 1 2020, 12:42 PM

mhorne added inline comments.

share/man/man9/getenv.9
216	Is there a preferred way to represent string literals in mdoc?

allanjude added a subscriber: larus.maxwell_netapp.com.Sep 1 2020, 4:06 PM

This is probably fine as-is. Having a getenv_bool means you could report EINVAL errors for unrecognized string values, but adds more work to callers. It would perhaps be nice to structure the wrapper routines as as wrappers around a getenv_bool and have the wrapper routines complain for EINVAL errors ("Environment variable %s has a non-boolean value %s" or the like).

In D26270#584275, @jhb wrote:

This is probably fine as-is. Having a getenv_bool means you could report EINVAL errors for unrecognized string values, but adds more work to callers. It would perhaps be nice to structure the wrapper routines as as wrappers around a getenv_bool and have the wrapper routines complain for EINVAL errors ("Environment variable %s has a non-boolean value %s" or the like).

I like the idea, but having getenv_bool return an error deviates from the other getenv_type functions, for which a non-zero value indicates success. I worry that it would lead to misuse.

I may still try this but keep getenv_bool static, since printing the warnings might have value wrt D26271, in case someone did something stupid like loader_env.disable=2.

Here's a version that also provides getenv_bool/TUNABLE_BOOL.

At first, I thought it would be odd to provide both getenv_is_true/false and
getenv_bool, but they do serve distinct purposes. If the caller only needs
the result once, such as in D26271, then getenv_is_true saves a few lines. If
we're looking at tunables, TUNABLE_BOOL provides the same semantics as
TUNABLE_INT but also allows parsing strings.

I'd like to hear your thoughts before making further updates to the manpages.

Harbormaster completed remote builds in B33509: Diff 76923.Sep 11 2020, 6:03 PM

This seems reasonable, though getenv_bool is defined static but declared/used as if it were not. :-)

Remove unneeded static from getenv_bool.

Harbormaster completed remote builds in B33511: Diff 76926.Sep 11 2020, 6:21 PM

Update manpage bits.

Harbormaster completed remote builds in B33597: Diff 77110.Sep 16 2020, 6:05 PM

jhb accepted this revision.Sep 17 2020, 5:19 PM

This revision is now accepted and ready to land.Sep 17 2020, 5:19 PM

rscheff added a subscriber: rscheff.Sep 18 2020, 2:28 PM

Closed by commit rS365945: Add getenv(9) boolean parsing functions (authored by mhorne). · Explain WhySep 21 2020, 3:25 PM

This revision was automatically updated to reflect the committed changes.