Re: [PATCHv2] support ZSTD compression algorithm

["Martin Liška" <mliska@suse.cz>]

Hi.

>> +  if (use_zstd)
>> +#ifdef USE_ZSTD
>> +    return __libelf_compress_zstd (scn, hsize, ei_data, orig_size,
>> +				   orig_addralign, new_size, force,
>> +				   data, next_data, out_buf, out_size,
>> +				   block);
>> +#else
>> +    return NULL;
> 
> Should this also call __libelf_seterrno (ELF_E_UNKNOWN_COMPRESSION_TYPE) ?

Yes, will fix that.

> 
>> +#endif
>> +  else
>> +    return __libelf_compress_zlib (scn, hsize, ei_data, orig_size,
>> +				   orig_addralign, new_size, force,
>> +				   data, next_data, out_buf, out_size,
>> +				   block);
>> +}
>> +
>> +void *
>> +internal_function
>> +__libelf_decompress_zlib (void *buf_in, size_t size_in, size_t size_out)
>>   {
>>     /* Catch highly unlikely compression ratios so we don't allocate
>>        some giant amount of memory for nothing. The max compression
>> @@ -218,7 +360,7 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
>>         return NULL;
>>       }
>> -  /* Malloc might return NULL when requestion zero size.  This is highly
>> +  /* 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.  */
> 
> OK, typo fix.
> 
>> @@ -260,6 +402,51 @@ __libelf_decompress (void *buf_in, size_t size_in, size_t size_out)
>>     return buf_out;
>>   }
>> +#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_DECOMPRESS_ERROR);
>> +      return NULL;
>> +    }
>> +  else
>> +    return buf_out;
>> +}
>> +#endif
> 
> OK.
> 
>> +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);
> 
> Maybe ELF_E_UNKNOWN_COMPRESSION_TYPE ?

Yes, will fix that.

> 
>> +    return NULL;
>> +#endif
>> +    }
>> +}
>> +
>>   void *
>>   internal_function
>>   __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
>> @@ -268,7 +455,7 @@ __libelf_decompress_elf (Elf_Scn *scn, size_t *size_out, size_t *addralign)
>>     if (gelf_getchdr (scn, &chdr) == NULL)
>>       return NULL;
>> -  if (chdr.ch_type != ELFCOMPRESS_ZLIB)
>> +  if (chdr.ch_type != ELFCOMPRESS_ZLIB && chdr.ch_type != ELFCOMPRESS_ZSTD)
>>       {
>>         __libelf_seterrno (ELF_E_UNKNOWN_COMPRESSION_TYPE);
>>         return NULL;
> 
> Should the chdr.ch_type != ELFCOMPRESS_ZSTD be guarded by #ifdef USE_ZSTD ?

Yes, will fix that.
>> +      else
>> +	error (0, 0, "Couldn't get chdr for section %zd", ndx);
> 
> Shouldn't this error be fatal?

What do you use for fatal errors?
> 
>> @@ -1378,7 +1404,7 @@ main (int argc, char **argv)
>>   	N_("Place (de)compressed output into FILE"),
>>   	0 },
>>         { "type", 't', "TYPE", 0,
>> -	N_("What type of compression to apply. TYPE can be 'none' (decompress), 'zlib' (ELF ZLIB compression, the default, 'zlib-gabi' is an alias) or 'zlib-gnu' (.zdebug GNU style compression, 'gnu' is an alias)"),
>> +	N_("What type of compression to apply. TYPE can be 'none' (decompress), 'zlib' (ELF ZLIB compression, the default, 'zlib-gabi' is an alias), 'zlib-gnu' (.zdebug GNU style compression, 'gnu' is an alias) or 'zstd'"),
>>   	0 },
>>         { "name", 'n', "SECTION", 0,
>>   	N_("SECTION name to (de)compress, SECTION is an extended wildcard pattern (defaults to '.?(z)debug*')"),
> 
> I would say or 'zstd' (ELF ZSTD compression)" to match the 'zlib; type
> description.

Works for me.

Cheers,
Martin

> 
>> 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.  */
> 
> OK.
> 
>> diff --git a/tests/run-compress-test.sh b/tests/run-compress-test.sh
>> index a6a298f5..3f9c990e 100755
>> --- a/tests/run-compress-test.sh
>> +++ b/tests/run-compress-test.sh
>> @@ -61,6 +61,30 @@ testrun_elfcompress_file()
>>       echo "uncompress $elfcompressedfile -> $elfuncompressedfile"
>>       testrun ${abs_top_builddir}/src/elfcompress -v -t none -o ${elfuncompressedfile} ${elfcompressedfile}
>>       testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${elfuncompressedfile}
>> +
>> +    outputfile="${infile}.gabi.zstd"
>> +    tempfiles "$outputfile"
>> +    echo "zstd compress $elfcompressedfile -> $outputfile"
>> +    testrun ${abs_top_builddir}/src/elfcompress -v -t zstd -o ${outputfile} ${elfcompressedfile}
>> +    testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${outputfile}
>> +    echo "checking compressed section header" $outputfile
>> +    testrun ${abs_top_builddir}/src/readelf -Sz ${outputfile} | grep "ELF ZSTD" >/dev/null
>> +
>> +    zstdfile="${infile}.zstd"
>> +    tempfiles "$zstdfile"
>> +    echo "zstd compress $uncompressedfile -> $zstdfile"
>> +    testrun ${abs_top_builddir}/src/elfcompress -v -t zstd -o ${zstdfile} ${elfuncompressedfile}
>> +    testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${zstdfile}
>> +    echo "checking compressed section header" $zstdfile
>> +    testrun ${abs_top_builddir}/src/readelf -Sz ${zstdfile} | grep "ELF ZSTD" >/dev/null
>> +
>> +    zstdgnufile="${infile}.zstd.gnu"
>> +    tempfiles "$zstdgnufile"
>> +    echo "zstd re-compress to GNU ZLIB $zstdfile -> $zstdgnufile"
>> +    testrun ${abs_top_builddir}/src/elfcompress -v -t zlib-gnu -o ${zstdgnufile} ${zstdfile}
>> +    testrun ${abs_top_builddir}/src/elfcmp ${uncompressedfile} ${zstdgnufile}
>> +    echo "checking .zdebug section name" $zstdgnufile
>> +    testrun ${abs_top_builddir}/src/readelf -S ${zstdgnufile} | grep ".zdebug" >/dev/null
>>   }
>>   testrun_elfcompress()
> 
> You might add these to a separate run test file or pass some ZSTD flag
> through the test environment to conditionally run these new tests.
> 
> Cheers,
> 
> Mark