svn commit: r367813 - head/lib/libutil

[Stefan Esser]

Am 18.11.20 um 22:40 schrieb Mateusz Guzik:
>> +{
>> +	static const int localbase_oid[2] = {CTL_USER, USER_LOCALBASE};
> 
> There is no use for this to be static.

Why not? This makes it part of the constant initialized data of
the library, which is more efficient under run-time and memory
space aspects than initializing them on the stack.

What do I miss?

>> +	char *tmppath;
>> +	size_t tmplen;
>> +	static const char *localbase = NULL;
>> +
>> +	if (issetugid() == 0) {
>> +		tmppath = getenv("LOCALBASE");
>> +		if (tmppath != NULL && tmppath[0] != '\0')
>> +			return (tmppath);
>> +	}
>> +	if (sysctl(localbase_oid, 2, NULL, &tmplen, NULL, 0) == 0 &&
>> +	    (tmppath = malloc(tmplen)) != NULL &&
>> +	    sysctl(localbase_oid, 2, tmppath, &tmplen, NULL, 0) == 0) {
> 
> Apart from the concurrency issue mentioned in the comment this is just
> very wasteful. Instead you can have a small local buffer, say 128
> bytes and pass that to be populated. The sysctl handler than can
> populate that and return an error if the size is too small. I don't
> know if sysclt api allows it to return the set size as it is. Worst
> case you can just retry with a bigger malloced buffer.

You obviously did not follow the development of this code in the
review. It used to have such a buffer, but this was rejected, since
only very few of the programs that link against this library are
going to call this function.

Allocating a small buffer and replacing it with a larger one if it
was too small adds needless complexity to this program and needs more
code. It is not sensible (IMHO) to reduce the number of system calls
by 1 for the small number of programs that use this feature, and which
perform at least tens of other system calls at start-up.

> Once you get the result you can malloc a buffer and
> atomic_cmpset_rel_ptr localbase to point to it. If this fails, another
> thread got the result, you free your buffer and return (localbase).

Yes, I wrote that I could use atomics, feel free to modify the program
accordingly. It will not make a difference in practice, since there is
no good reason to call this function from within a multi-threaded
context at all, and none of the candidates to be modified to use this
function in base do.

> Also the kernel counter part completely unnecessarily comes with a
> static 1KB buffer to hold what typically is going to be nothing. This
> should also be allocated as needed. Worst case you can add a trivial
> lock around it.

The kernel buffer does already exist and you did not complain about
that buffer, when it has been committed. A size of PATHNAMEMAX is
used to allow for any valid path name to be set from the loader.
If there is consensus that the value should be limited to e.g. 64 or
128 bytes, this is trivially changed in kern_mib.c.

I'd be more worried, if this buffer existed not as a single instance
in the kernel but e.g. per process, but this is not the case.

Feel free to make this a dynamically allocated variable, but make
sure that you do not increase the code size by as much as you reduce
the static storage required.

I do not need the dynamic assignment to localbase at all, and I have
suggested to introduce a kernel build option (e.g. WITH_LOCALBASE_VAR)
to control compilation of kernel and library with this function or to
return just the constant string _PATH_LOCALBASE or "/usr/local".

But there were very strong requests for this dynamically set localbase
and I have provided an implementation that is functional and easy to
use.

Regards, STefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20201118/2efc5fe5/attachment.sig>