Re: [PATCH v4 1/2] Import upstream zstd-1.3.6

[Nick Terrell <terrelln@fb.com>]

> On Nov 6, 2018, at 6:48 AM, Daniel Kiper <dkiper@net-space.pl> wrote:
> 
> On Wed, Oct 31, 2018 at 10:56:16AM -0700, Nick Terrell wrote:
>> Import zstd-1.3.6 from upstream [1]. Only the files need for decompression
>> are imported. Additionally makes zstd a module by adding module.c which
>> contains the license, and updates Makefile.core.def.
>> 
>> I used the latest zstd release, which includes patches [2] to build cleanly
>> in GRUB.
>> 
>> Upstream zstd commit hash: 4fa456d7f12f8b27bd3b2f5dfd4f46898cb31c24
>> Upstream zstd commit name: Merge pull request #1354 from facebook/dev
>> 
>> I've included the script used to import zstd-1.3.6 below.
>> 
>> [1] https://github.com/facebook/zstd/releases/tag/v1.3.6
>> [2] https://github.com/facebook/zstd/pull/1344
>> 
>> ```
>> #!/bin/sh -e
>> 
>> curl -L -O https://github.com/facebook/zstd/releases/download/v1.3.6/zstd-1.3.6.tar.gz
>> curl -L -O https://github.com/facebook/zstd/releases/download/v1.3.6/zstd-1.3.6.tar.gz.sha256
>> sha256sum --check zstd-1.3.6.tar.gz.sha256
>> tar xzf zstd-1.3.6.tar.gz
>> 
>> SRC_LIB="zstd-1.3.6/lib"
>> DST_LIB="grub-core/lib/zstd"
>> rm -rf $DST_LIB
>> mkdir -p $DST_LIB
>> cp $SRC_LIB/zstd.h $DST_LIB/
>> cp $SRC_LIB/common/*.[hc] $DST_LIB/
>> cp $SRC_LIB/decompress/*.[hc] $DST_LIB/
>> rm $DST_LIB/{pool.[hc],threading.[hc]}
>> rm -rf zstd-1.3.6*
>> echo SUCCESS!
>> ```
>> 
>> Signed-off-by: Nick Terrell <terrelln@fb.com>
> 
> I have a few concerns. First of all I can see that you include a lot of
> standard headers, e.g. stdlib.h, string.h, stddef.h, etc. Where do they
> come from? OS? If yes that is wrong. Additionally, I think that you
> should not use standard types, e.g. size_t, because this may not work if
> you cross compile. You have to use, e.g. grub_size_t. IMO the simplest
> solution would be definition and usage of relevant zstd_* types, e.g.
> zstd_size_t. Then e.g. zstd_size_t should be defined as grub_size_t.

I'm getting the standard headers from grub-core/lib/posix_wrap. I need
to add stddef.h. I'd like to keep using standard types, which are typedefed
to the grub_ equivalent in posix_wrap/sys/types.h.

I agree it would be nice if zstd put all of its dependencies into a configuration
file that we could replace. I'll look into adding this in a future version. However,
since we are using upstream zstd as-is right now, and posix_wrapper is already
present, I think that the benefit of using upstream as-is outweighs the cost of
using the posix_wrapper. Does that sound reasonable to you?

To test that I'm using the right headers, I looked that the .Po files generated,
and saw that stddef.h was using the system, but the rest were using the
posix_wrapper. Is that the right approach? Is there something else I should
be testing?

> [...]

Nick