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
Date: Fri, 5 Apr 2024 11:29:05 +0200 [thread overview]
Message-ID: <Zg_EYX5m-GTyfPbY@wunner.de> (raw)
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
next prev parent reply other threads:[~2024-04-05 9:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-20 15:10 [PATCH v3] X.509: Introduce scope-based x509_certificate allocation Lukas Wunner
2024-02-20 18:00 ` Jarkko Sakkinen
2024-02-20 18:03 ` Jarkko Sakkinen
2024-04-05 9:29 ` Lukas Wunner [this message]
2024-03-01 10:10 ` Herbert Xu
2024-03-02 8:27 ` Lukas Wunner
2024-03-04 8:03 ` Herbert Xu
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=Zg_EYX5m-GTyfPbY@wunner.de \
--to=lukas@wunner.de \
--cc=Jonathan.Cameron@huawei.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=ardb@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=herbert@gondor.apana.org.au \
--cc=jarkko@kernel.org \
--cc=keyrings@vger.kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=peterz@infradead.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).