> On Nov 6, 2018, at 6:48 AM, Daniel Kiper <dkiper@xxxxxxxxxxxx> 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@xxxxxx> > > 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
