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

[Daniel Kiper <dkiper@net-space.pl>]

On Tue, Nov 06, 2018 at 07:43:04PM +0000, Nick Terrell wrote:
> > 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?

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