| Message ID | 20181031175617.230241-1-terrelln@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: Add zstd support to grub btrfs | expand |
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. [...] > 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
> 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
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
Hi all, This patch set imports the upstream zstd library, adds zstd support to the btrfs module, and adds a test case. I've also tested the patch set by storing my boot partition in btrfs with and without zstd compression and rebooting. The fist patch imports the files needed to support zstd decompression from zstd-1.3.6 as-is. It is a very large patch. In case it doesn't make it, I've included the commit hash and the script I used to download the files. Upstream zstd commit hash: 4fa456d7f12f8b27bd3b2f5dfd4f46898cb31c24 Upstream zstd commit name: Merge pull request #1354 from facebook/dev ``` #!/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! ``` Best, Nick Terrell Changelog: v1 -> v2: - Switch to upstream zstd-1.3.6 and drop all the local patches. - Fix comments from Daniel Kiper. v2 -> v3: - Remove an extra file accidentally included in the first patch. - Use grub_error() to set grub_errno in grub_btrfs_zstd_decompress(). - Fix style and formatting comments. v3 -> v4: - Put zstd in its own module. - Update commit messages. - Use attribute unused. - Rebase on top of RAID patchset. Nick Terrell (2): Import upstream zstd-1.3.6 btrfs: Add zstd support to grub btrfs Makefile.util.def | 10 +- grub-core/Makefile.core.def | 17 +- grub-core/fs/btrfs.c | 108 +- grub-core/lib/zstd/bitstream.h | 458 ++++ grub-core/lib/zstd/compiler.h | 133 ++ grub-core/lib/zstd/cpu.h | 215 ++ grub-core/lib/zstd/debug.c | 44 + grub-core/lib/zstd/debug.h | 123 + grub-core/lib/zstd/entropy_common.c | 236 ++ grub-core/lib/zstd/error_private.c | 48 + grub-core/lib/zstd/error_private.h | 76 + grub-core/lib/zstd/fse.h | 708 ++++++ grub-core/lib/zstd/fse_decompress.c | 309 +++ grub-core/lib/zstd/huf.h | 334 +++ grub-core/lib/zstd/huf_decompress.c | 1096 +++++++++ grub-core/lib/zstd/mem.h | 374 ++++ grub-core/lib/zstd/module.c | 3 + grub-core/lib/zstd/xxhash.c | 876 ++++++++ grub-core/lib/zstd/xxhash.h | 305 +++ grub-core/lib/zstd/zstd.h | 1516 +++++++++++++ grub-core/lib/zstd/zstd_common.c | 81 + grub-core/lib/zstd/zstd_decompress.c | 3108 ++++++++++++++++++++++++++ grub-core/lib/zstd/zstd_errors.h | 92 + grub-core/lib/zstd/zstd_internal.h | 257 +++ tests/btrfs_test.in | 1 + tests/util/grub-fs-tester.in | 2 +- 26 files changed, 10526 insertions(+), 4 deletions(-) create mode 100644 grub-core/lib/zstd/bitstream.h create mode 100644 grub-core/lib/zstd/compiler.h create mode 100644 grub-core/lib/zstd/cpu.h create mode 100644 grub-core/lib/zstd/debug.c create mode 100644 grub-core/lib/zstd/debug.h create mode 100644 grub-core/lib/zstd/entropy_common.c create mode 100644 grub-core/lib/zstd/error_private.c create mode 100644 grub-core/lib/zstd/error_private.h create mode 100644 grub-core/lib/zstd/fse.h create mode 100644 grub-core/lib/zstd/fse_decompress.c create mode 100644 grub-core/lib/zstd/huf.h create mode 100644 grub-core/lib/zstd/huf_decompress.c create mode 100644 grub-core/lib/zstd/mem.h create mode 100644 grub-core/lib/zstd/module.c create mode 100644 grub-core/lib/zstd/xxhash.c create mode 100644 grub-core/lib/zstd/xxhash.h create mode 100644 grub-core/lib/zstd/zstd.h create mode 100644 grub-core/lib/zstd/zstd_common.c create mode 100644 grub-core/lib/zstd/zstd_decompress.c create mode 100644 grub-core/lib/zstd/zstd_errors.h create mode 100644 grub-core/lib/zstd/zstd_internal.h -- 2.17.1