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

[Daniel Kiper]

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 <address@hidden>

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.

[...]

> diff --git a/grub-core/lib/zstd/module.c b/grub-core/lib/zstd/module.c
> new file mode 100644
> index 000000000..e4d0cace8
> --- /dev/null
> +++ b/grub-core/lib/zstd/module.c
> @@ -0,0 +1,3 @@

Could you copy the header e.g. from grub-core/loader/multiboot_mbi2.c?
Of course you have to update the dates accordingly.

> +#include <grub/dl.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3");

Daniel