Re: [PATCH 1/2] zfs: Support zstd compression

[Daniel Kiper]

On Thu, May 16, 2024 at 10:42:25PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
>  grub-core/Makefile.core.def |  1 +
>  grub-core/fs/zfs/zfs.c      | 32 ++++++++++++++++++++++++++++++++
>  include/grub/zfs/zio.h      |  1 +
>  3 files changed, 34 insertions(+)
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8e1b1d9f3..2ba4962d5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1596,6 +1596,7 @@ module = {
>    common = fs/zfs/zfs_lz4.c;
>    common = fs/zfs/zfs_sha256.c;
>    common = fs/zfs/zfs_fletcher.c;
> +  cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/zstd';
>  };
>
>  module = {
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index b5453e006..b8441faef 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -57,6 +57,8 @@
>  #include <grub/i18n.h>
>  #include <grub/safemath.h>
>
> +#include <zstd.h>
> +
>  GRUB_MOD_LICENSE ("GPLv3+");
>
>  #define      ZPOOL_PROP_BOOTFS               "bootfs"
> @@ -291,6 +293,7 @@ static const char *spa_feature_names[] = {
>    "com.delphix:embedded_data",
>    "com.delphix:extensible_dataset",
>    "org.open-zfs:large_blocks",
> +  "org.freebsd:zstd_compress",
>    NULL
>  };
>
> @@ -312,6 +315,34 @@ zlib_decompress (void *s, void *d,
>    return grub_errno;
>  }
>
> +static grub_err_t
> +zstd_decompress (void *ibuf, void *obuf, grub_size_t isize,
> +              grub_size_t osize)
> +{
> +  grub_size_t zstd_ret;
> +  grub_uint8_t *byte_buf = (grub_uint8_t *) ibuf;
> +
> +  if (isize < 8)
> +      return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data too 
> short");
> +
> +  grub_uint32_t c_len = grub_be_to_cpu32(grub_get_unaligned32(byte_buf));

May I ask you to define c_len at the beginning of the function?
And please fix coding style for function calls.

> +  if (c_len > isize - 8)
> +      return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data announced 
> size overflow");
> +
> +  /* Fix magic number.  */

I think the fix should be explained why it is needed.

> +  byte_buf[4] = 0x28;
> +  byte_buf[5] = 0xb5;
> +  byte_buf[6] = 0x2f;
> +  byte_buf[7] = 0xfd;
> +  zstd_ret = ZSTD_decompress (obuf, osize, byte_buf + 4, c_len + 4);
> +
> +  if (ZSTD_isError (zstd_ret))
> +    return grub_error (GRUB_ERR_BAD_COMPRESSED_DATA, "zstd data corrupted 
> (error %d)", (int) zstd_ret);
> +
> +  return GRUB_ERR_NONE;
> +}

Daniel