On Wed, Oct 31, 2018 at 10:56:17AM -0700, Nick Terrell wrote:
> 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 <terre...@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';
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.
[...]
> +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?
> + }
If above is OK then drop this curly brackets.
> + 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
