| Message ID | 20220228160827.465488-1-gitter.spiros@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: Use GLIBC_TUNABLES instead of MALLOC_CHECK_ on glibc >= 2.34 | expand |
Elia Pinto <gitter.spiros@gmail.com> writes: > In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment > variables have been replaced by GLIBC_TUNABLES. Also the new Does it hurt to have these older environment variables? If not, we would prefer to see redundant but less deeply indented code, I would imagine. > + if type -p getconf >/dev/null 2>&1; then > + _GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')" > + if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then > + _HAVE_GLIBC_234="yes" > + fi > + fi Style. We prefer "test ..." over "[ ... ]" and more importantly we don't use "test X -a Y". Do we absolutely need "test -p getconf" with an extra indentation? I suspect we don't. if _GLIBC_VERSION=$(getconf GNU_LIBC_VERSION 2>/dev/null) && _GLIBC_VERSION=${_GLIBC_VERSION#"glibc "} && test 2.34 \<= "$_GLIBC_VERSION" then USE_GLIBC_TUNABLES=YesPlease fi perhaps? I am not sure if glibc 2.4 still matters, getconf reports it as 2.04 or 2.4, for the above comparison to be OK, though. In any case, HAVE_GLIBC_234 is a horrible variable name to use for this purpose, as the primary thing the two use sites care about is not the version but if they should use the GLIBC_TUNABLES mechanism, so it would be better to name the variable after the feature. > setup_malloc_check () { > - MALLOC_CHECK_=3 MALLOC_PERTURB_=165 > - export MALLOC_CHECK_ MALLOC_PERTURB_ > + if test "x$_HAVE_GLIBC_234" = xyes ; then > + LD_PRELOAD="libc_malloc_debug.so.0" GLIBC_TUNABLES="glibc.malloc.check=1:glibc.malloc.perturb=165" > + export LD_PRELOAD GLIBC_TUNABLES > + else > + MALLOC_CHECK_=3 MALLOC_PERTURB_=165 > + export MALLOC_CHECK_ MALLOC_PERTURB_ > + fi Avoid overly long lines when you can easily do so. MALLOC_CHECK_=3 MALLOC_PERTURB_=165 export MALLOC_CHECK_ MALLOC_PERTURB_ + case "$USE_GLIBC_TUNABLES" in + YesPlease) + g= + LD_PRELOAD=libc_malloc_debug.so.0 + for t in \ + glibc.malloc.check=1 \ + glibc.malloc.perturb=165 \ + do + g="$g${g:+:}$t" + done + GLIBC_TUNABLES=$g + ;; + esac perhaps? > } > teardown_malloc_check () { > - unset MALLOC_CHECK_ MALLOC_PERTURB_ > + if test "x$_HAVE_GLIBC_234" = xyes ; then > + unset LD_PRELOAD GLIBC_TUNABLES > + else > + unset MALLOC_CHECK_ MALLOC_PERTURB_ > + fi Similarly. > } > fi
On 2022-02-28 at 19:13:40, Junio C Hamano wrote: > Elia Pinto <gitter.spiros@gmail.com> writes: > > > In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment > > variables have been replaced by GLIBC_TUNABLES. Also the new > > Does it hurt to have these older environment variables? If not, > we would prefer to see redundant but less deeply indented code, > I would imagine. Setting both sets of environment variables is probably just fine and the simplest solution, I'd imagine. > > + if type -p getconf >/dev/null 2>&1; then > > + _GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')" > > + if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then > > + _HAVE_GLIBC_234="yes" > > + fi > > + fi > > Style. We prefer "test ..." over "[ ... ]" and more importantly we > don't use "test X -a Y". > > Do we absolutely need "test -p getconf" with an extra indentation? > I suspect we don't. getconf is specified by POSIX, but that doesn't mean it's in the default install or in PATH on all systems. However, we should write "command -v getconf" instead if we need to check, since that's the POSIX way to write it, and "type" is not guaranteed to be present on all systems. It looks like the code might silently not match if getconf isn't present, and if so then we can avoid the check, but we should of course put a comment noting that behavior to help future readers.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: >> > + if type -p getconf >/dev/null 2>&1; then >> > + _GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')" >> > + if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then >> > + _HAVE_GLIBC_234="yes" >> > + fi >> > + fi >> >> Style. We prefer "test ..." over "[ ... ]" and more importantly we >> don't use "test X -a Y". >> >> Do we absolutely need "test -p getconf" with an extra indentation? >> I suspect we don't. > > getconf is specified by POSIX, but that doesn't mean it's in the default > install or in PATH on all systems. However, we should write "command -v > getconf" instead if we need to check, since that's the POSIX way to > write it, and "type" is not guaranteed to be present on all systems. My point was that the code relies on having getconf on PATH anyway, so it is sufficient to attempt running getconf and using its output after checking it begins with "glibc". Missing getconf or getconf that is different from what we expect it to be would be rejected by the same code, without needing the above nested if .. if .. fi .. fi
diff --git a/t/test-lib.sh b/t/test-lib.sh index e4716b0b86..136614ac8c 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -517,12 +517,27 @@ then : nothing } else + if type -p getconf >/dev/null 2>&1; then + _GLIBC_VERSION="$(getconf GNU_LIBC_VERSION 2>/dev/null | awk '{ print $2 }')" + if [ -n "$_GLIBC_VERSION" -a $(expr "$_GLIBC_VERSION" \>= "2.34") ]; then + _HAVE_GLIBC_234="yes" + fi + fi setup_malloc_check () { - MALLOC_CHECK_=3 MALLOC_PERTURB_=165 - export MALLOC_CHECK_ MALLOC_PERTURB_ + if test "x$_HAVE_GLIBC_234" = xyes ; then + LD_PRELOAD="libc_malloc_debug.so.0" GLIBC_TUNABLES="glibc.malloc.check=1:glibc.malloc.perturb=165" + export LD_PRELOAD GLIBC_TUNABLES + else + MALLOC_CHECK_=3 MALLOC_PERTURB_=165 + export MALLOC_CHECK_ MALLOC_PERTURB_ + fi } teardown_malloc_check () { - unset MALLOC_CHECK_ MALLOC_PERTURB_ + if test "x$_HAVE_GLIBC_234" = xyes ; then + unset LD_PRELOAD GLIBC_TUNABLES + else + unset MALLOC_CHECK_ MALLOC_PERTURB_ + fi } fi
In glibc >= 2.34 MALLOC_CHECK_ and MALLOC_PERTURB_ environment variables have been replaced by GLIBC_TUNABLES. Also the new glibc requires that you preload a library called libc_malloc_debug.so to get these features. Using the ordinary glibc system variable detect if this is glibc >= 2.34 and use GLIBC_TUNABLES and the new library. This patch was inspired by a Richard W.M. Jones ndbkit patch Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> --- t/test-lib.sh | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)