[REVIEW] new function getlocalbase() - D27236 and D27237

From: Stefan Esser <se_at_freebsd.org>
Date: Mon, 16 Nov 2020 18:00:41 +0100
I have created two Phabricator reviews for my proposed implementation
of getlocalbase():

	https://reviews.freebsd.org/D27236
	https://reviews.freebsd.org/D27237

The first one implements getlocalbase() with quite similar semantics
to getenv("LOCALBASE") which it will replace in a number of places in
the base system.

This implementation returns a pointer to either of:

1) getenv("LOCALBASE")
2) sysctlbyname("user.localbase", ...)
3) _PATH_LOCALBASE or "/usr/local"

I had considered to copy any result of 1) or 3) into the same static
buffer used to retrieve the sysctl result, but have for now implemented
a version that returns the pointers as is (in case of getenv() into the
environment, in case of the fall-back strings into the area for R/O
strings).

I'd be willing to change this to either:

a) retrieve a value on the first invocation and copy it to the buffer
b) retrieve a new value on each invocation and copy it to the buffer

Most programs will call getlocalbase() at most once and will either
construct the required path to e.g. a config file directory from it,
or they will store a local copy. The return type should prevent any
accidental overwriting of values at the returned address (but does
not really protect e.g. the environment variable area - same as if
getenv() was directly called).

If returning the pointer into the environment is considered too
dangerous, I'd prefer to implement variant a).

A potential problem exists due the unlimited length of the string
returned by getenv("LOCALBASE"), i.e. it could cause a path name
longer than PATHNAMEMAX to be created. I do not want to introduce
a potential error return or to silently discard superfluous data
from the returned value and therefore prefer to return the pointer
into the environment area without guarantees regarding the maximum
length of the string pointed to.


The second review replaces getenv() calls with getlocalbase() in
code that already used getenv(). The code is simplified but has
unchanged behavior if LOCALBASE is defined in the environment.
If it is undefined, than the sysctl value or the hard-coded value
is returned (and the only difference is that sysctl may cause the
system wide value to be controlled without recompilation of the
world).

In one program the constant _PATH_LOCALBASE was concatenated to
a relative file path, and in that case the same approach can be
used as in the other two (with snprintf() filling a local buffer).

I have not looked for other programs that should be modified to
use getlocalbase(), but all affected by my recent _PATH_LOCALBASE
commit are candidates ...

I'd appreciate comments in the Phabricator or review or by mail.

Regards, STefan

Received on Mon Nov 16 2020 - 16:00:45 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:41:25 UTC