linux-kernel - Re: [PATCH v5 1/9] lib: zstd: Add zstd compatibility wrapper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Nov 2020 16:25:41 +0100
From:   David Sterba <dsterba@...e.cz>
To:     Chris Mason <clm@...com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Nick Terrell <nickrterrell@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        linux-crypto@...r.kernel.org, linux-btrfs@...r.kernel.org,
        squashfs-devel@...ts.sourceforge.net,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, Kernel Team <Kernel-team@...com>,
        Nick Terrell <terrelln@...com>, Petr Malat <oss@...at.biz>,
        Johannes Weiner <jweiner@...com>,
        Niket Agarwal <niketa@...com>, Yann Collet <cyan@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v5 1/9] lib: zstd: Add zstd compatibility wrapper

On Mon, Nov 09, 2020 at 02:01:41PM -0500, Chris Mason wrote:
> On 6 Nov 2020, at 13:38, Christoph Hellwig wrote:
> > You just keep resedning this crap, don't you?  Haven't you been told
> > multiple times to provide a proper kernel API by now?
> 
> You do consistently ask for a shim layer, but you haven’t explained 
> what we gain by diverging from the documented and tested API of the 
> upstream zstd project.  It’s an important discussion given that we 
> hope to regularly update the kernel side as they make improvements in 
> zstd.
> 
> The only benefit described so far seems to be camelcase related, but if 
> there are problems in the API beyond that, I haven’t seen you describe 
> them.  I don’t think the camelcase alone justifies the added costs of 
> the shim.

The API change in this patchset is adding churn that wouldn't be
necessary if there were an upstream<->kernel API from the beginning.

The patch 5/9 is almost entirely renaming just some internal identifiers

-			      ZSTD_CStreamWorkspaceBound(params.cParams),
-			      ZSTD_DStreamWorkspaceBound(ZSTD_BTRFS_MAX_INPUT));
+			      ZSTD_estimateCStreamSize_usingCParams(params.cParams),
+			      ZSTD_estimateDStreamSize(ZSTD_BTRFS_MAX_INPUT));

plus updating the names in the error strings. The compression API that
filesystems need is simple:

- set up workspace and parameters
- compress buffer
- decompress buffer

We really should not care if upstream has 3 functions for initializing
stream (ZSTD_initCStream/ZSTD_initStaticCStream/ZSTD_initCStream_advanced),
or if the name changes again in the future.

This should not require explicit explanation, this should be a natural
requirement especially for separate projects that don't share the same
coding style but have to be integrated in some way.