svn commit: r367813 - head/lib/libutil

[Stefan Esser]

Am 19.11.20 um 01:37 schrieb Mateusz Guzik:
> On 11/19/20, Stefan Esser <se at freebsd.org> wrote:
[...]
>> I just wanted to provide an implementation of this functionality to
>> be used in a number of programs where other developers had expressed
>> interest in such a feature (and one of these programs has been worked
>> on by me in recent weeks, so I'm now able to make use of it myself).
> 
> The entire localbase saga is getting way out of hand.

Yes, apparently.

> To address this e-mail and things you wrote in another reply, my
> comlaints are very simple and are not getting less valid for not being
> raised sooner. I just was not watching any of this until recent
> fallout.
> 
> For the change at hand, there has to be a reason to use a static
> symbol. Standard example is catching otherwise expensive to obtain
> data. For that the static localbase pointer makes perfect sense, while
> the static lookup array does not have any justification that I see.

The reason to use a static symbol for the return value is that I do
not want to have to use the getlocalbase() on my systems at all.

The static lookup array is trivially changed to be stack allocated
and filled at run-time, this is a non-issue, IMHO.


As explained in an earlier discussion, I'd rather build my systems
with a _PATH_LOCALBASE set to a non-default value than use any
run-time setting of this value. And thus, my implementation will
be just to define getlocalbase in the libutil.h on my systems:

#define getlocalbase() _PATH_LOCALBASE

Copying into a user provided buffer is of course possible, and such
an implementation had been committed before. It does not allow to
enforce a compiled in _PATH_LOCALBASE in the way I want to use it,
but I would find a way around this.

But the diffs to programs that use getlocalbase with caller supplied
buffers were significantly larger. My version can just replace the
getenv("LOCALBASE") found in a number of places by getlocalbase()
(and since getlocalbase() returns the default value if the environment
variable has not been set, remove the fallback code for that case).

> Bringing cache, TLB or whatever microarchitectural details into the
> discussion is beyond not warranted.
I did not start bringing in such issues, see the mail I responded to.
And I argued that it just did not matter which way the argument array
was defined, since either method has minor advantages and disadvantages.

> More importantly though the commit comes with a self-confessed memory
> leak and is a show stopper.

There is no memory leak, actually. Only if you called getlocalbase()
in a multi-threaded environment multiple times and in such a way that
the non-NULL test before the assignment overlaps with an assignment
in another thread, there could have been a leak of a few bytes. But
you could not exploit this leak in any way, since there is a limited
number of cores that can execute threads in parallel.

My supposed initial version did not have any memory leak and had been
rejected due to the pre-allocation of a static buffer. I do not agree
that this is an issue, since the number of VM pages allocated for the
data segment of the library does not grow, and this is what counts.

It is easily possible to limit the user.localbase variable to a useful
size in the kernel and the library (e.g. 64 bytes, which should be
sufficient as a PREFIX). But even at MAXPATHLEN, the memory usage is
not increased by this buffer.

> That said, I'll see about patching this up.

I have created a new review with a static buffer. It does not have any
memory leaks, does not increase the memory usage of libutil, and it is
fully thread safe and guaranteed to return the same value on each
successive invocation (which seems to make sense for a system parameter
like LOCALBASE - the code can easily be changed to perform the getenv
and sysctl calls on each invocation again).

https://reviews.freebsd.org/D27370

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-all/attachments/20201125/1f51b135/attachment.sig>