Received-SPF: pass (google.com: domain of linux-crypto+bounces-3366-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 5 Apr 2024 11:29:05 +0200
From: Lukas Wunner <lukas@wunner.de>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: David Howells <dhowells@redhat.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH v3] X.509: Introduce scope-based x509_certificate
 allocation
Message-ID: <Zg_EYX5m-GTyfPbY@wunner.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CZA3R5R9CVYD.1HH1S662FW2RX@seitikki>
 <CZA3PCY3U4YU.3R05ZC4X16EX0@seitikki>

On Tue, Feb 20, 2024 at 06:00:41PM +0000, Jarkko Sakkinen wrote:
> On Tue Feb 20, 2024 at 3:10 PM UTC, Lukas Wunner wrote:
> > Add a DEFINE_FREE() clause for x509_certificate structs and use it in
> > x509_cert_parse() and x509_key_preparse().  These are the only functions
> > where scope-based x509_certificate allocation currently makes sense.
> > A third user will be introduced with the forthcoming SPDM library
> > (Security Protocol and Data Model) for PCI device authentication.
> 
> I think you are adding scope-based memory management and not
> DEFINE_FREE(). Otherwise, this would be one-liner patch.

Above it states very clearly: "and use it in x509_cert_parse() and
x509_key_preparse()".

That's the reason it is not a one-liner patch.


> I'm not sure if the last sentence adds more than clutter as this
> patch has nothing to do with SPDM changes per se.

I disagree.  It is important as a justification for the change that
the two functions converted here are not going to be the only users,
but that there's a third one coming up.


> > I've compared the Assembler output before/after and they are identical,
> > save for the fact that gcc-12 always generates two return paths when
> > __cleanup() is used, one for the success case and one for the error case.
> 
> Use passive as commit message is not a personal letter.

Okay, I will respin and change to passive mood.


> I don't see a story here but instead I see bunch of disordered tecnical
> terms.

That doesn't sound like constructive criticism.


> We have the code diff for detailed technical stuff. The commit message
> should simply explain why we want this and what it does for us.
[...]
> What is the most important function of a commit message? Well, it comes
> when the commit is in the mainline. It reminds of the *reasons* why a
> change was made and this commit message does not really serve well in
> that role.

The reason for the commit is that Jonathan requested it during code
review of my PCI device authentication patches.  I've been stating
this very clearly in the first iteration of the present patch.
You asked that I delete the sentence and instead use a Suggested-by.

Perhaps it would have been better had I not listened to you.
Because now you seem to have forgotten the reason for the patch,
which, again, you asked me to delete.

FWIW, here's the link to Jonathan's review:
https://lore.kernel.org/all/20231003153937.000034ca@Huawei.com/

And here's the quote with his explicit request to use cleanup.h:

   "Maybe cleanup.h magic?  Seems like it would simplify error paths here a
    tiny bit. Various other cases follow, but I won't mention this every time
    [...]
    Even this could be done with the cleanup.h stuff with appropriate
    pointer stealing and hence allow direct returns.
    This is the sort of case that I think really justifies that stuff."

Thanks,

Lukas