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. BR, Jarkko
