From: Mark Wielaard <mark@klomp.org>
To: "Martin Liška" <mliska@suse.cz>, elfutils-devel@sourceware.org
Subject: Re: [PATCHv2] support ZSTD compression algorithm
Date: Thu, 22 Dec 2022 19:36:26 +0100 [thread overview]
Message-ID: <dce6e062c326b8cf6d7c3a004d4a32a0ffc63156.camel@klomp.org> (raw)
In-Reply-To: <0703880d-6648-866e-7fbc-3f5d6b18f49d@suse.cz>
Hi Martin,
On Thu, 2022-12-22 at 10:17 +0100, Martin Liška wrote:
> Is the abort () at the second call site because that cannot happen?
> > Or should that also goto cleanup?
>
> 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" = xyes], [LIBLZMA="liblzma"],
> [LIBLZMA=""])
> 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" = xyes], [LIBZSTD="libzstd"],
> [LIBLZSTD=""])
> AC_SUBST([LIBZSTD])
> +zstd_LIBS="$LIBS"
> +AC_SUBST([zstd_LIBS])
> zip_LIBS="$LIBS"
> LIBS="$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 <string.h>
> #include <zlib.h>
>
> +#ifdef USE_ZSTD
> +#include <zstd.h>
> +#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)
>
> -/* 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.
> [...]
> +#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 = malloc (size_out ?: 1);
> + if (unlikely (buf_out == NULL))
> + {
> + __libelf_seterrno (ELF_E_NOMEM);
> + return NULL;
> + }
> +
> + size_t ret = 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 == 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 = -1,
> NONE,
> ZLIB,
> + ZSTD,
>
> /* Maximal supported ch_type. */
> - MAXIMAL_CH_TYPE = ZLIB,
> + MAXIMAL_CH_TYPE = ZSTD,
>
> ZLIB_GNU = 1 << 16
> };
> @@ -139,6 +140,12 @@ parse_opt (int key, char *arg __attribute__
> ((unused)),
> type = ZLIB;
> else if (strcmp ("zlib-gnu", arg) == 0 || strcmp ("gnu", arg)
> == 0)
> type = ZLIB_GNU;
> + else if (strcmp ("zstd", arg) == 0)
> +#ifdef USE_ZSTD
> + type = 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 = elf_compress (scn, dchtype, flags);
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 == 0)
> - return "NONE";
> -
> - if (code == 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";
> + }
> }
>
> /* 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
>
> if HAVE_ZSTD
> TESTS += run-readelf-compressed-zstd.sh
> +export ELFUTILS_ZSTD = 1
> endif
So this is the wrong guard. HAVE_ZSTD is whether we have the zstd
binary or not. Guard export ELFUTILS_ZSTD = 1 with USE_ZSTD_COMPRESS.
Thanks,
Mark
next prev parent reply other threads:[~2022-12-22 18:36 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-24 11:09 [PATCH][RFC] readelf: partial support of ZSTD compression Martin Liška
2022-10-24 11:41 ` Dmitry V. Levin
2022-10-24 12:17 ` Martin Liška
2022-10-24 16:48 ` Dmitry V. Levin
2022-10-24 18:16 ` Martin Liška
2022-10-28 22:21 ` Mark Wielaard
2022-11-28 13:16 ` Martin Liška
2022-11-28 22:29 ` Mark Wielaard
2022-11-29 9:34 ` Martin Liška
2022-11-29 12:05 ` [PATCHv2] support ZSTD compression algorithm Martin Liška
2022-12-09 10:17 ` Martin Liška
2022-12-15 13:17 ` Mark Wielaard
2022-12-19 14:19 ` Martin Liška
2022-12-19 15:09 ` Mark Wielaard
2022-12-21 11:13 ` Martin Liška
2023-01-10 17:44 ` [PATCH] readelf: Check compression status of .debug section data Mark Wielaard
2023-01-16 19:39 ` Mark Wielaard
2022-12-16 0:32 ` [PATCHv2] support ZSTD compression algorithm Mark Wielaard
2022-12-19 14:21 ` Martin Liška
2022-12-19 15:16 ` Mark Wielaard
2022-12-21 11:09 ` Martin Liška
2022-12-21 23:14 ` Mark Wielaard
2022-12-22 9:17 ` Martin Liška
2022-12-22 18:36 ` Mark Wielaard [this message]
2022-12-23 8:44 ` Martin Liška
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dce6e062c326b8cf6d7c3a004d4a32a0ffc63156.camel@klomp.org \
--to=mark@klomp.org \
--cc=elfutils-devel@sourceware.org \
--cc=mliska@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).