From: Nick Terrell <terrelln@fb.com>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: Nick Terrell <nickrterrell@gmail.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
Btrfs BTRFS <linux-btrfs@vger.kernel.org>,
"squashfs-devel@lists.sourceforge.net"
<squashfs-devel@lists.sourceforge.net>,
"linux-f2fs-devel@lists.sourceforge.net"
<linux-f2fs-devel@lists.sourceforge.net>,
LKML <linux-kernel@vger.kernel.org>,
Kernel Team <Kernel-team@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
Date: Thu, 3 Dec 2020 01:42:03 +0000 [thread overview]
Message-ID: <297D9C8B-5F4D-4E3B-A5FD-DA292D8BA12A@fb.com> (raw)
In-Reply-To: <20201203011606.GA20621@qmqm.qmqm.pl>
> On Dec 2, 2020, at 5:16 PM, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>
> 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.
Good point. The zstd API no longer exposes the error code enum,
but it does expose zstd_get_error_name() which can be used here.
I was thinking that the string needed to be static for some reason, but
that is not the case. I will make that change.
>> +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?
If breaking strict aliasing and type-punning by pointer casing is okay, then
we can do that here. These memcpys will be negligible for performance, but
type-punning would be more succinct if allowed.
Best,
Nick
next prev parent reply other threads:[~2020-12-03 1:44 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-02 20:32 [PATCH v6 0/3] Update to zstd-1.4.6 Nick Terrell
2020-12-02 20:32 ` [PATCH v6 1/3] lib: zstd: Add kernel-specific API Nick Terrell
2020-12-03 1:16 ` Michał Mirosław
2020-12-03 1:42 ` Nick Terrell [this message]
2020-12-03 3:14 ` Michał Mirosław
2020-12-03 3:59 ` Nick Terrell
2020-12-03 5:03 ` Michał Mirosław
2020-12-03 5:59 ` Nick Terrell
2020-12-03 20:50 ` Nick Terrell
2020-12-02 20:32 ` [PATCH v6 2/3] lib: zstd: Add decompress_sources.h for decompress_unzstd Nick Terrell
2020-12-02 20:32 ` [PATCH v6 3/3] lib: zstd: Upgrade to latest upstream zstd version 1.4.6 Nick Terrell
2020-12-02 23:58 ` kernel test robot
2020-12-04 14:03 ` David Sterba
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=297D9C8B-5F4D-4E3B-A5FD-DA292D8BA12A@fb.com \
--to=terrelln@fb.com \
--cc=Kernel-team@fb.com \
--cc=clm@fb.com \
--cc=cyan@fb.com \
--cc=hch@infradead.org \
--cc=herbert@gondor.apana.org.au \
--cc=jweiner@fb.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=mirq-linux@rere.qmqm.pl \
--cc=nickrterrell@gmail.com \
--cc=niketa@fb.com \
--cc=oss@malat.biz \
--cc=squashfs-devel@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).