Re: [PATCH v4 2/2] btrfs: Add zstd support to grub btrfs

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

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 <terrelln@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