On Tue Feb 20, 2024 at 6:00 PM UTC, 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. > > I'm not sure if the last sentence adds more than clutter as this > patch has nothing to do with SPDM changes per se. > > > Unlike most other DEFINE_FREE() clauses, this one checks for IS_ERR() > > instead of NULL before calling x509_free_certificate() at end of scope. > > That's because the "constructor" of x509_certificate structs, > > x509_cert_parse(), returns a valid pointer or an ERR_PTR(), but never > > NULL. > > > > 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. > > > > > In x509_cert_parse(), add a hint for the compiler that kzalloc() never > > returns an ERR_PTR(). Otherwise the compiler adds a gratuitous IS_ERR() > > check on return. Introduce a handy assume() macro for this which can be > > re-used elsewhere in the kernel to provide hints for the compiler. > > Does not explain why it is "handy". > > I don't see a story here but instead I see bunch of disordered tecnical > terms. > > 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. And we > zero care about PCI changes in the scope of this patch, especially since > this is not part of such patch set. I mean think it this way. 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. BR, Jarkko
