Page MenuHomeFreeBSD

Add getenv_is_true() and getenv_is_false()
ClosedPublic

Authored by mhorne on Sep 1 2020, 12:40 PM.

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mhorne created this revision.
mhorne added inline comments.
share/man/man9/getenv.9
216 ↗(On Diff #76473)

Is there a preferred way to represent string literals in mdoc?

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.

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

Remove unneeded static from getenv_bool.

This revision is now accepted and ready to land.Sep 17 2020, 5:19 PM
This revision was automatically updated to reflect the committed changes.