Discussion:
[PATCH v4 0/2] btrfs: Add zstd support to grub btrfs
(too old to reply)
Nick Terrell
2018-10-31 17:56:15 UTC
Permalink
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
Nick Terrell
2018-10-31 17:56:17 UTC
Permalink
Adds zstd support to the btrfs module.

Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
compression. A test case was also added to the test suite that fails before
the patch, and passes after.

Signed-off-by: Nick Terrell <***@fb.com>
---
v1 -> v2:
- Fix comments from Daniel Kiper.

v2 -> v3:
- Use grub_error() to set grub_errno in grub_btrfs_zstd_decompress().
- Fix style and formatting comments.

v3 -> v4:
- Use attribute unused.

Makefile.util.def | 10 +++-
grub-core/Makefile.core.def | 2 +-
grub-core/fs/btrfs.c | 108 ++++++++++++++++++++++++++++++++++-
tests/btrfs_test.in | 1 +
tests/util/grub-fs-tester.in | 2 +-
5 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/Makefile.util.def b/Makefile.util.def
index 9ae45f351..3ce9388ae 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -54,7 +54,7 @@ library = {
library = {
name = libgrubmods.a;
cflags = '-fno-builtin -Wno-undef';
- cppflags = '-I$(top_srcdir)/grub-core/lib/minilzo -I$(srcdir)/grub-core/lib/xzembed -DMINILZO_HAVE_CONFIG_H';
+ cppflags = '-I$(srcdir)/grub-core/lib/minilzo -I$(srcdir)/grub-core/lib/xzembed -I$(srcdir)/grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H';

common_nodist = grub_script.tab.c;
common_nodist = grub_script.yy.c;
@@ -165,6 +165,14 @@ library = {
common = grub-core/lib/xzembed/xz_dec_bcj.c;
common = grub-core/lib/xzembed/xz_dec_lzma2.c;
common = grub-core/lib/xzembed/xz_dec_stream.c;
+ common = grub-core/lib/zstd/debug.c;
+ common = grub-core/lib/zstd/entropy_common.c;
+ common = grub-core/lib/zstd/error_private.c;
+ common = grub-core/lib/zstd/fse_decompress.c;
+ common = grub-core/lib/zstd/huf_decompress.c;
+ common = grub-core/lib/zstd/xxhash.c;
+ common = grub-core/lib/zstd/zstd_common.c;
+ common = grub-core/lib/zstd/zstd_decompress.c;
};

program = {
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 540a31f78..b09319328 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1280,7 +1280,7 @@ module = {
common = fs/btrfs.c;
common = lib/crc.c;
cflags = '$(CFLAGS_POSIX) -Wno-undef';
- cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo -DMINILZO_HAVE_CONFIG_H';
+ cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/minilzo -I$(srcdir)/lib/zstd -DMINILZO_HAVE_CONFIG_H';
};

module = {
diff --git a/grub-core/fs/btrfs.c b/grub-core/fs/btrfs.c
index cac9ef588..8cfb997c5 100644
--- a/grub-core/fs/btrfs.c
+++ b/grub-core/fs/btrfs.c
@@ -17,6 +17,8 @@
* along with GRUB. If not, see <http://www.gnu.org/licenses/>.
*/

+#define ZSTD_STATIC_LINKING_ONLY
+
#include <grub/err.h>
#include <grub/file.h>
#include <grub/mm.h>
@@ -27,6 +29,7 @@
#include <grub/lib/crc.h>
#include <grub/deflate.h>
#include <minilzo.h>
+#include <zstd.h>
#include <grub/i18n.h>
#include <grub/btrfs.h>
#include <grub/crypto.h>
@@ -47,6 +50,9 @@ GRUB_MOD_LICENSE ("GPLv3+");
#define GRUB_BTRFS_LZO_BLOCK_MAX_CSIZE (GRUB_BTRFS_LZO_BLOCK_SIZE + \
(GRUB_BTRFS_LZO_BLOCK_SIZE / 16) + 64 + 3)

+#define ZSTD_BTRFS_MAX_WINDOWLOG 17
+#define ZSTD_BTRFS_MAX_INPUT (1 << ZSTD_BTRFS_MAX_WINDOWLOG)
+
typedef grub_uint8_t grub_btrfs_checksum_t[0x20];
typedef grub_uint16_t grub_btrfs_uuid_t[8];

@@ -217,6 +223,7 @@ struct grub_btrfs_extent_data
#define GRUB_BTRFS_COMPRESSION_NONE 0
#define GRUB_BTRFS_COMPRESSION_ZLIB 1
#define GRUB_BTRFS_COMPRESSION_LZO 2
+#define GRUB_BTRFS_COMPRESSION_ZSTD 3

#define GRUB_BTRFS_OBJECT_ID_CHUNK 0x100

@@ -1216,6 +1223,91 @@ grub_btrfs_read_inode (struct grub_btrfs_data *data,
return grub_btrfs_read_logical (data, elemaddr, inode, sizeof (*inode), 0);
}

+static void* grub_zstd_malloc (void* state __attribute__((unused)), size_t size)
+{
+ return grub_malloc (size);
+}
+
+static void grub_zstd_free (void* state __attribute__((unused)), void* address)
+{
+ return grub_free (address);
+}
+
+static ZSTD_customMem grub_zstd_allocator (void)
+{
+ ZSTD_customMem allocator;
+
+ allocator.customAlloc = &grub_zstd_malloc;
+ allocator.customFree = &grub_zstd_free;
+ allocator.opaque = NULL;
+
+ return allocator;
+}
+
+static grub_ssize_t
+grub_btrfs_zstd_decompress (char *ibuf, grub_size_t isize, grub_off_t off,
+ char *obuf, grub_size_t osize)
+{
+ void *allocated = NULL;
+ char *otmpbuf = obuf;
+ grub_size_t otmpsize = osize;
+ ZSTD_DCtx *dctx = NULL;
+ grub_size_t zstd_ret;
+ grub_ssize_t ret = -1;
+
+ /*
+ * Zstd will fail if it can't fit the entire output in the destination
+ * buffer, so if osize isn't large enough, allocate a temporary buffer.
+ */
+ if (otmpsize < ZSTD_BTRFS_MAX_INPUT) {
+ allocated = grub_malloc (ZSTD_BTRFS_MAX_INPUT);
+ if (!allocated) {
+ goto err;
+ }
+ otmpbuf = (char*)allocated;
+ otmpsize = ZSTD_BTRFS_MAX_INPUT;
+ }
+
+ /* Create the ZSTD_DCtx. */
+ dctx = ZSTD_createDCtx_advanced (grub_zstd_allocator ());
+ if (!dctx) {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "zstd out of memory");
+ goto err;
+ }
+
+ /*
+ * Get the real input size, there may be junk at the
+ * end of the frame.
+ */
+ isize = ZSTD_findFrameCompressedSize (ibuf, isize);
+ if (ZSTD_isError (isize)) {
+ grub_error (GRUB_ERR_BAD_COMPRESSED_DATA,
+ "zstd data corrupted");
+ goto err;
+ }
+
+ /* Decompress and check for errors. */
+ zstd_ret = ZSTD_decompressDCtx (dctx, otmpbuf, otmpsize, ibuf, isize);
+ if (ZSTD_isError (zstd_ret)) {
+ grub_error (GRUB_ERR_BAD_COMPRESSED_DATA,
+ "zstd data corrupted");
+ goto err;
+ }
+
+ /*
+ * Move the requested data into the obuf. obuf may be equal
+ * to otmpbuf, which is why grub_memmove() is required.
+ */
+ grub_memmove (obuf, otmpbuf + off, osize);
+ ret = osize;
+
+err:
+ grub_free (allocated);
+ ZSTD_freeDCtx (dctx);
+
+ return ret;
+}
+
static grub_ssize_t
grub_btrfs_lzo_decompress(char *ibuf, grub_size_t isize, grub_off_t off,
char *obuf, grub_size_t osize)
@@ -1391,7 +1483,8 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,

if (data->extent->compression != GRUB_BTRFS_COMPRESSION_NONE
&& data->extent->compression != GRUB_BTRFS_COMPRESSION_ZLIB
- && data->extent->compression != GRUB_BTRFS_COMPRESSION_LZO)
+ && data->extent->compression != GRUB_BTRFS_COMPRESSION_LZO
+ && data->extent->compression != GRUB_BTRFS_COMPRESSION_ZSTD)
{
grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
"compression type 0x%x not supported",
@@ -1431,6 +1524,15 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
!= (grub_ssize_t) csize)
return -1;
}
+ else if (data->extent->compression == GRUB_BTRFS_COMPRESSION_ZSTD)
+ {
+ if (grub_btrfs_zstd_decompress(data->extent->inl, data->extsize -
+ ((grub_uint8_t *) data->extent->inl
+ - (grub_uint8_t *) data->extent),
+ extoff, buf, csize)
+ != (grub_ssize_t) csize)
+ return -1;
+ }
else
grub_memcpy (buf, data->extent->inl + extoff, csize);
break;
@@ -1468,6 +1570,10 @@ grub_btrfs_extent_read (struct grub_btrfs_data *data,
ret = grub_btrfs_lzo_decompress (tmp, zsize, extoff
+ grub_le_to_cpu64 (data->extent->offset),
buf, csize);
+ else if (data->extent->compression == GRUB_BTRFS_COMPRESSION_ZSTD)
+ ret = grub_btrfs_zstd_decompress (tmp, zsize, extoff
+ + grub_le_to_cpu64 (data->extent->offset),
+ buf, csize);
else
ret = -1;

diff --git a/tests/btrfs_test.in b/tests/btrfs_test.in
index 2b37ddd33..0c9bf3a68 100644
--- a/tests/btrfs_test.in
+++ b/tests/btrfs_test.in
@@ -18,6 +18,7 @@ fi
"@builddir@/grub-fs-tester" btrfs
"@builddir@/grub-fs-tester" btrfs_zlib
"@builddir@/grub-fs-tester" btrfs_lzo
+"@builddir@/grub-fs-tester" btrfs_zstd
"@builddir@/grub-fs-tester" btrfs_raid0
"@builddir@/grub-fs-tester" btrfs_raid1
"@builddir@/grub-fs-tester" btrfs_single
diff --git a/tests/util/grub-fs-tester.in b/tests/util/grub-fs-tester.in
index ef65fbc93..bc14a05ca 100644
--- a/tests/util/grub-fs-tester.in
+++ b/tests/util/grub-fs-tester.in
@@ -629,7 +629,7 @@ for LOGSECSIZE in $(range "$MINLOGSECSIZE" "$MAXLOGSECSIZE" 1); do
;;
x"btrfs")
"mkfs.btrfs" -s $SECSIZE -L "$FSLABEL" "${MOUNTDEVICE}" ;;
- x"btrfs_zlib" | x"btrfs_lzo")
+ x"btrfs_zlib" | x"btrfs_lzo" | x"btrfs_zstd")
"mkfs.btrfs" -s $SECSIZE -L "$FSLABEL" "${MOUNTDEVICE}"
MOUNTOPTS="compress=${fs/btrfs_/},"
MOUNTFS="btrfs"
--
2.17.1
Daniel Kiper
2018-11-06 15:06:34 UTC
Permalink
Post by Nick Terrell
Adds zstd support to the btrfs module.
Tested on Ubuntu-18.04 with a btrfs /boot partition with and without zstd
compression. A test case was also added to the test suite that fails before
the patch, and passes after.
---
- Fix comments from Daniel Kiper.
- Use grub_error() to set grub_errno in grub_btrfs_zstd_decompress().
- Fix style and formatting comments.
- Use attribute unused.
Makefile.util.def | 10 +++-
grub-core/Makefile.core.def | 2 +-
grub-core/fs/btrfs.c | 108 ++++++++++++++++++++++++++++++++++-
tests/btrfs_test.in | 1 +
tests/util/grub-fs-tester.in | 2 +-
5 files changed, 119 insertions(+), 4 deletions(-)
diff --git a/Makefile.util.def b/Makefile.util.def
index 9ae45f351..3ce9388ae 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -54,7 +54,7 @@ library = {
library = {
name = libgrubmods.a;
cflags = '-fno-builtin -Wno-undef';
- cppflags = '-I$(top_srcdir)/grub-core/lib/minilzo -I$(srcdir)/grub-core/lib/xzembed -DMINILZO_HAVE_CONFIG_H';
+ cppflags = '-I$(srcdir)/grub-core/lib/minilzo -I$(srcdir)/grub-core/lib/xzembed -I$(srcdir)/grub-core/lib/zstd -DMINILZO_HAVE_CONFIG_H';
I am not strongly against s/top_srcdir/srcdir/ here. However, if you do
such things I will ask you to add a blurb why to the commit message.

[...]
Post by Nick Terrell
+static grub_ssize_t
+grub_btrfs_zstd_decompress (char *ibuf, grub_size_t isize, grub_off_t off,
+ char *obuf, grub_size_t osize)
+{
+ void *allocated = NULL;
+ char *otmpbuf = obuf;
+ grub_size_t otmpsize = osize;
+ ZSTD_DCtx *dctx = NULL;
+ grub_size_t zstd_ret;
+ grub_ssize_t ret = -1;
+
+ /*
+ * Zstd will fail if it can't fit the entire output in the destination
+ * buffer, so if osize isn't large enough, allocate a temporary buffer.
+ */
+ if (otmpsize < ZSTD_BTRFS_MAX_INPUT) {
+ allocated = grub_malloc (ZSTD_BTRFS_MAX_INPUT);
+ if (!allocated) {
+ goto err;
Hmmm... You fail silently. Should not you say something here?
Post by Nick Terrell
+ }
If above is OK then drop this curly brackets.
Post by Nick Terrell
+ otmpbuf = (char*)allocated;
+ otmpsize = ZSTD_BTRFS_MAX_INPUT;
+ }
+
+ /* Create the ZSTD_DCtx. */
+ dctx = ZSTD_createDCtx_advanced (grub_zstd_allocator ());
+ if (!dctx) {
+ grub_error (GRUB_ERR_OUT_OF_MEMORY, "zstd out of memory");
...and here you complain. OK, but why GRUB_ERR_OUT_OF_MEMORY?
This probably applies to grub_zstd_allocator() but maybe not to
ZSTD_createDCtx_advanced(). AIUI both functions may return
different errors.

Daniel
Nick Terrell
2018-10-31 17:56:16 UTC
Permalink
This post might be inappropriate. Click to display it.
Daniel Kiper
2018-11-06 14:48:32 UTC
Permalink
Post by Nick Terrell
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!
```
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.

[...]
Post by Nick Terrell
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.
Post by Nick Terrell
+#include <grub/dl.h>
+
+GRUB_MOD_LICENSE ("GPLv3");
Daniel
Nick Terrell
2018-11-06 19:43:04 UTC
Permalink
Post by Daniel Kiper
Post by Nick Terrell
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!
```
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?
Post by Daniel Kiper
[...]
Nick
Daniel Kiper
2018-11-07 11:08:05 UTC
Permalink
Post by Nick Terrell
Post by Daniel Kiper
Post by Nick Terrell
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!
```
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

Loading...