Re: [PATCHv2] support ZSTD compression algorithm

[Mark Wielaard <mark@klomp.org>]

Hi Martin,

On Tue, 2022-11-29 at 13:05 +0100, Martin Liška wrote:
> There's second version of the patch that fully support both
> compression and decompression.
> 
> Changes from the v1:
> - compression support added
> - zstd detection is fixed
> - new tests are added
> - builds fine w/ and w/o the ZSTD library
> 
> What's currently missing and where I need a help:
> 
> 1) When I build ./configure --without-zstd, I don't have
> a reasonable error message (something similar to binutils's readelf:
> readelf: Warning: section '.debug_str' has unsupported compress type:
> 2)
> even though, __libelf_decompress returns NULL and __libelf_seterrno).
> One can see a garbage in the console.
> 
> How to handle that properly?

Is there a particular way you are running eu-readelf? Is it with
generic -w or -a, or decoding a specific section type?

> 2) How should I run my newly added tests conditionally if zstd
> configuration support is enabled?

eu_ZIP will define an AM_CONDITIONAL for ZSTD (note this is different
from the HAVE_ZSTD, which is the am conditional added for having the
zstd program itself). You could use it in tests/Makefile.am as:

  if ZSTD
  TESTS += run-zstd-compress-test.sh
  endif

If you had a separate test... Otherwise add some variable to
TESTS_ENVIRONMENT (and installed_TESTS_ENVIRONMENT) based on the Make
variable that is tested inside the shell script (somewhat like
USE_VALGRIND) maybe.

>   configure.ac               |   8 +-
>   libelf/Makefile.am         |   2 +-
>   libelf/elf_compress.c      | 294 ++++++++++++++++++++++++++++++--
> -----
>   libelf/elf_compress_gnu.c  |   5 +-
>   libelf/libelfP.h           |   4 +-
>   src/elfcompress.c          | 144 ++++++++++--------
>   src/readelf.c              |  18 ++-
>   tests/run-compress-test.sh |  24 +++
>   8 files changed, 373 insertions(+), 126 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 59be27ac..07cfa54b 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -410,6 +410,11 @@ dnl Test for bzlib and xz/lzma/zstd, gives BZLIB/LZMALIB/ZSTD .am
>   dnl conditional and config.h USE_BZLIB/USE_LZMALIB/USE_ZSTD #define.
>   save_LIBS="$LIBS"
>   LIBS=
> +eu_ZIPLIB(zstd,ZSTD,zstd,ZSTD_decompress,[ZSTD (zst)])
> +AS_IF([test "x$with_zstd" = xyes], [LIBZSTD="libzstd"], [LIBLZSTD=""])
> +AC_SUBST([LIBZSTD])
> +zstd_LIBS="$LIBS"
> +AC_SUBST([zstd_LIBS])
>   eu_ZIPLIB(bzlib,BZLIB,bz2,BZ2_bzdopen,bzip2)
>   # We need this since bzip2 doesn't have a pkgconfig file.
>   BZ2_LIB="$LIBS"
> @@ -417,9 +422,6 @@ AC_SUBST([BZ2_LIB])
>   eu_ZIPLIB(lzma,LZMA,lzma,lzma_auto_decoder,[LZMA (xz)])
>   AS_IF([test "x$with_lzma" = xyes], [LIBLZMA="liblzma"], [LIBLZMA=""])
>   AC_SUBST([LIBLZMA])
> -eu_ZIPLIB(zstd,ZSTD,zstd,ZSTD_decompress,[ZSTD (zst)])
> -AS_IF([test "x$with_zstd" = xyes], [LIBZSTD="libzstd"], [LIBLZSTD=""])
> -AC_SUBST([LIBZSTD])
>   zip_LIBS="$LIBS"
>   LIBS="$save_LIBS"
>   AC_SUBST([zip_LIBS])

Doing AC_SUBST([zstd_LIBS]) seems correct. Why is the test moved
earlier?

You are testing for ZSTD_decompress, is that enough? Asking because I
see you are using ZSTD_compressStream2, which seems to requires libzstd
v1.4.0+.

In general how stable is the libzstd api?

You'll also need to use the AC_SUBST for LIBZSTD in config/libelf.pc.in
now, as used in the libdw.pc.in:

  Requires.private: zlib @LIBZSTD@

> diff --git a/libelf/Makefile.am b/libelf/Makefile.am
> index 560ed45f..24c25cf8 100644
> --- a/libelf/Makefile.am
> +++ b/libelf/Makefile.am
> @@ -106,7 +106,7 @@ libelf_pic_a_SOURCES =
>   am_libelf_pic_a_OBJECTS = $(libelf_a_SOURCES:.c=.os)
>   
>   libelf_so_DEPS = ../lib/libeu.a
> -libelf_so_LDLIBS = $(libelf_so_DEPS) -lz
> +libelf_so_LDLIBS = $(libelf_so_DEPS) -lz $(zstd_LIBS)
>   if USE_LOCKS
>   libelf_so_LDLIBS += -lpthread
>   endif

OK.

Haven't read the actual code yet. I'll get back to that later today.

Cheers,

Mark