On Tue, Nov 06, 2018 at 07:43:04PM +0000, Nick Terrell wrote: > > 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? If you are able to assure me that you are not pulling OS stuff into GRUB2 binary then I am OK with that. However, I would like to see a few words about the issue in the commit message. Daniel
