Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 3 Dec 2020 02:16:06 +0100
From:   =?iso-8859-2?Q?Micha=B3_Miros=B3aw?= <mirq-linux@rere.qmqm.pl>
To:     Nick Terrell <nickrterrell@gmail.com>
Cc:     Herbert Xu <herbert@gondor.apana.org.au>,
        linux-crypto@vger.kernel.org, linux-btrfs@vger.kernel.org,
        squashfs-devel@lists.sourceforge.net,
        linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, Kernel Team <Kernel-team@fb.com>,
        Nick Terrell <terrelln@fb.com>, Chris Mason <clm@fb.com>,
        Petr Malat <oss@malat.biz>, Johannes Weiner <jweiner@fb.com>,
        Niket Agarwal <niketa@fb.com>, Yann Collet <cyan@fb.com>,
        Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH v6 1/3] lib: zstd: Add kernel-specific API
Message-ID: <20201203011606.GA20621@qmqm.qmqm.pl>
References: <20201202203242.1187898-1-nickrterrell@gmail.com>
 <20201202203242.1187898-2-nickrterrell@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-2
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20201202203242.1187898-2-nickrterrell@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Wed, Dec 02, 2020 at 12:32:40PM -0800, Nick Terrell wrote:
> From: Nick Terrell <terrelln@fb.com>
> 
> This patch:
> - Moves `include/linux/zstd.h` -> `lib/zstd/zstd.h`
> - Adds a new API in `include/linux/zstd.h` that is functionally
>   equivalent to the in-use subset of the current API. Functions are
>   renamed to avoid symbol collisions with zstd, to make it clear it is
>   not the upstream zstd API, and to follow the kernel style guide.
> - Updates all callers to use the new API.
> 
> There are no functional changes in this patch. Since there are no
> functional change, I felt it was okay to update all the callers in a
> single patch, since once the API is approved, the callers are
> mechanically changed.
[...]
> --- a/lib/decompress_unzstd.c
> +++ b/lib/decompress_unzstd.c
[...]
>  static int INIT handle_zstd_error(size_t ret, void (*error)(char *x))
>  {
> -	const int err = ZSTD_getErrorCode(ret);
> -
> -	if (!ZSTD_isError(ret))
> +	if (!zstd_is_error(ret))
>  		return 0;
>  
> -	switch (err) {
> -	case ZSTD_error_memory_allocation:
> -		error("ZSTD decompressor ran out of memory");
> -		break;
> -	case ZSTD_error_prefix_unknown:
> -		error("Input is not in the ZSTD format (wrong magic bytes)");
> -		break;
> -	case ZSTD_error_dstSize_tooSmall:
> -	case ZSTD_error_corruption_detected:
> -	case ZSTD_error_checksum_wrong:
> -		error("ZSTD-compressed data is corrupt");
> -		break;
> -	default:
> -		error("ZSTD-compressed data is probably corrupt");
> -		break;
> -	}
> +	error("ZSTD decompression failed");
>  	return -1;
>  }

This looses diagnostics specificity - is this intended? At least the
out-of-memory condition seems useful to distinguish.

> +size_t zstd_compress_stream(zstd_cstream *cstream,
> +	struct zstd_out_buffer *output, struct zstd_in_buffer *input)
> +{
> +	ZSTD_outBuffer o;
> +	ZSTD_inBuffer i;
> +	size_t ret;
> +
> +	memcpy(&o, output, sizeof(o));
> +	memcpy(&i, input, sizeof(i));
> +	ret = ZSTD_compressStream(cstream, &o, &i);
> +	memcpy(output, &o, sizeof(o));
> +	memcpy(input, &i, sizeof(i));
> +	return ret;
> +}

Is all this copying necessary? How is it different from type-punning by
direct pointer cast?

Best Regards
Micha? Miros?aw