From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id C22F63858425 for ; Thu, 22 Dec 2022 18:36:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C22F63858425 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 81023300071A; Thu, 22 Dec 2022 19:36:26 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 41DEC413CD0E; Thu, 22 Dec 2022 19:36:26 +0100 (CET) Message-ID: Subject: Re: [PATCHv2] support ZSTD compression algorithm From: Mark Wielaard To: Martin =?UTF-8?Q?Li=C5=A1ka?= , elfutils-devel@sourceware.org Date: Thu, 22 Dec 2022 19:36:26 +0100 In-Reply-To: <0703880d-6648-866e-7fbc-3f5d6b18f49d@suse.cz> References: <0703880d-6648-866e-7fbc-3f5d6b18f49d@suse.cz> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-3038.5 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Martin, On Thu, 2022-12-22 at 10:17 +0100, Martin Li=C5=A1ka wrote: > Is the abort () at the second call site because that cannot happen? > > Or should that also goto cleanup? >=20 > Yes, it can't happen. Aha, because it should already have seen that section before, in which case it would have errored out. > There's V2 (including ChangeLog) where all issues apart from the > libzstd configure.ac detection should be addressed. See also my other email. I think that addresses everything. Just merge those and push please. > diff --git a/configure.ac b/configure.ac > index 59be27ac..ef16f79e 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -417,9 +417,11 @@ AC_SUBST([BZ2_LIB]) > eu_ZIPLIB(lzma,LZMA,lzma,lzma_auto_decoder,[LZMA (xz)]) > AS_IF([test "x$with_lzma" =3D xyes], [LIBLZMA=3D"liblzma"], > [LIBLZMA=3D""]) > AC_SUBST([LIBLZMA]) > -eu_ZIPLIB(zstd,ZSTD,zstd,ZSTD_decompress,[ZSTD (zst)]) > +eu_ZIPLIB(zstd,ZSTD,zstd,ZSTD_compressStream2,[ZSTD (zst)]) > AS_IF([test "x$with_zstd" =3D xyes], [LIBZSTD=3D"libzstd"], > [LIBLZSTD=3D""]) > AC_SUBST([LIBZSTD]) > +zstd_LIBS=3D"$LIBS" > +AC_SUBST([zstd_LIBS]) > zip_LIBS=3D"$LIBS" > LIBS=3D"$save_LIBS" > AC_SUBST([zip_LIBS]) See the other email for an idea how to keep most of the checks the same, but just for decompress (in the case of zstd). Then define a USE_ZSTD_COMPRESS only for new enough libzstd. > diff --git a/libelf/elf_compress.c b/libelf/elf_compress.c > index d7f53af2..deb585b7 100644 > --- a/libelf/elf_compress.c > +++ b/libelf/elf_compress.c > @@ -39,6 +39,10 @@ > #include > #include > =20 > +#ifdef USE_ZSTD > +#include > +#endif > + > /* Cleanup and return result. Don't leak memory. */ > static void * > do_deflate_cleanup (void *result, z_stream *z, void *out_buf, > @@ -54,53 +58,14 @@ do_deflate_cleanup (void *result, z_stream *z, > void *out_buf, > #define deflate_cleanup(result, cdata) \ > do_deflate_cleanup(result, &z, out_buf, cdata) > =20 > -/* Given a section, uses the (in-memory) Elf_Data to extract the > - original data size (including the given header size) and data > - alignment. Returns a buffer that has at least hsize bytes (for > the > - caller to fill in with a header) plus zlib compressed date. Also > - returns the new buffer size in new_size (hsize + compressed data > - size). Returns (void *) -1 when FORCE is false and the > compressed > - data would be bigger than the original data. */ > void * > internal_function > -__libelf_compress (Elf_Scn *scn, size_t hsize, int ei_data, > - size_t *orig_size, size_t *orig_addralign, > - size_t *new_size, bool force) > +__libelf_compress_zlib (Elf_Scn *scn, size_t hsize, int ei_data, > + size_t *orig_size, size_t *orig_addralign, > + size_t *new_size, bool force, > + Elf_Data *data, Elf_Data *next_data, > + void *out_buf, size_t out_size, size_t block) Missed this in my other patch, but __libelf_compress_zlib also isn't an internal_function, it can/must just be static. =20 > [...] > +#ifdef USE_ZSTD So this is the compression part, guard that with USE_ZSTD_COMPRESS. > + > void * > internal_function > -__libelf_decompress (void *buf_in, size_t size_in, size_t size_out) > +__libelf_decompress_zlib (void *buf_in, size_t size_in, size_t > size_out) > { So this isn't an internal function, just mark it static void *. > +#ifdef USE_ZSTD > +void * > +internal_function > +__libelf_decompress_zstd (void *buf_in, size_t size_in, size_t > size_out) > +{ > + /* Malloc might return NULL when requesting zero size. This is > highly > + unlikely, it would only happen when the compression was forced. > + But we do need a non-NULL buffer to return and set as result. > + Just make sure to always allocate at least 1 byte. */ > + void *buf_out =3D malloc (size_out ?: 1); > + if (unlikely (buf_out =3D=3D NULL)) > + { > + __libelf_seterrno (ELF_E_NOMEM); > + return NULL; > + } > + > + size_t ret =3D ZSTD_decompress (buf_out, size_out, buf_in, size_in); > + if (ZSTD_isError (ret)) > + { > + free (buf_out); > + __libelf_seterrno (ELF_E_UNKNOWN_COMPRESSION_TYPE); This should be ELF_E_DECOMPRESS_ERROR. > + return NULL; > + } > + else > + return buf_out; > +} > +#endif > + > +void * > +internal_function > +__libelf_decompress (int chtype, void *buf_in, size_t size_in, > size_t size_out) > +{ > + if (chtype =3D=3D ELFCOMPRESS_ZLIB) > + return __libelf_decompress_zlib (buf_in, size_in, size_out); > + else > + { > +#ifdef USE_ZSTD > + return __libelf_decompress_zstd (buf_in, size_in, size_out); > +#else > + __libelf_seterrno (ELF_E_DECOMPRESS_ERROR); And this should be ELF_E_UNKNOWN_COMPRESSION_TYPE. > + return NULL; > +#endif > + } > +} > diff --git a/src/elfcompress.c b/src/elfcompress.c > index eff765e8..bfdac2b4 100644 > --- a/src/elfcompress.c > +++ b/src/elfcompress.c > @@ -55,9 +55,10 @@ enum ch_type > UNSET =3D -1, > NONE, > ZLIB, > + ZSTD, > =20 > /* Maximal supported ch_type. */ > - MAXIMAL_CH_TYPE =3D ZLIB, > + MAXIMAL_CH_TYPE =3D ZSTD, > =20 > ZLIB_GNU =3D 1 << 16 > }; > @@ -139,6 +140,12 @@ parse_opt (int key, char *arg __attribute__ > ((unused)), > type =3D ZLIB; > else if (strcmp ("zlib-gnu", arg) =3D=3D 0 || strcmp ("gnu", arg) > =3D=3D 0) > type =3D ZLIB_GNU; > + else if (strcmp ("zstd", arg) =3D=3D 0) > +#ifdef USE_ZSTD > + type =3D ZSTD; > +#else > + argp_error (state, N_("ZSTD support is not enabled")); > +#endif This is slightly trickier now. I think you can use USE_ZSTD_COMPRESS as guard here, since this is always about compression. In the new setup you can have either no zstd support, only decompression support or both. But you can also just let elf_compress return the right error and make another change: diff --git a/src/elfcompress.c b/src/elfcompress.c index bfdac2b4..1f32331c 100644 --- a/src/elfcompress.c +++ b/src/elfcompress.c @@ -230,7 +230,8 @@ compress_section (Elf_Scn *scn, size_t orig_size, const char *name, res =3D elf_compress (scn, dchtype, flags); =20 if (res < 0) - error (0, 0, "Couldn't decompress section [%zd] %s: %s", + error (0, 0, "Couldn't %s section [%zd] %s: %s", + compress ? "compress" : "decompress", ndx, name, elf_errmsg (-1)); else { So the user knows whether it is compression or decompression that failed. > diff --git a/src/readelf.c b/src/readelf.c > index cc3e0229..451f8400 100644 > --- a/src/readelf.c > +++ b/src/readelf.c > @@ -1238,13 +1238,17 @@ get_visibility_type (int value) > static const char * > elf_ch_type_name (unsigned int code) > { > - if (code =3D=3D 0) > - return "NONE"; > - > - if (code =3D=3D ELFCOMPRESS_ZLIB) > - return "ZLIB"; > - > - return "UNKNOWN"; > + switch (code) > + { > + case 0: > + return "NONE"; > + case ELFCOMPRESS_ZLIB: > + return "ZLIB"; > + case ELFCOMPRESS_ZSTD: > + return "ZSTD"; > + default: > + return "UNKNOWN"; > + } > } > =20 > /* Print the section headers. */ > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 356b3fbf..c1868307 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -218,6 +218,7 @@ endif > =20 > if HAVE_ZSTD > TESTS +=3D run-readelf-compressed-zstd.sh > +export ELFUTILS_ZSTD =3D 1 > endif So this is the wrong guard. HAVE_ZSTD is whether we have the zstd binary or not. Guard export ELFUTILS_ZSTD =3D 1 with USE_ZSTD_COMPRESS. Thanks, Mark