Heinrich, On Thu, Apr 23, 2020 at 09:34:37AM +0200, Heinrich Schuchardt wrote: > On 23.04.20 02:28, AKASHI Takahiro wrote: > > Heinrich, > > > > On Tue, Apr 21, 2020 at 12:23:13PM +0200, Heinrich Schuchardt wrote: > >> On 4/21/20 2:37 AM, AKASHI Takahiro wrote: > >>> By adding extra symbols, we can now avoid including x509_parser and > >>> pkcs7_parser.h files multiple times. > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > >>> Suggested-by: Heinrich Schuchardt <xypron.g...@gmx.de> > >>> --- > >>> lib/crypto/pkcs7_parser.h | 4 ++++ > >>> lib/crypto/x509_parser.h | 4 ++++ > >>> lib/efi_loader/efi_image_loader.c | 1 + > >>> lib/efi_loader/efi_signature.c | 1 + > >>> 4 files changed, 10 insertions(+) > >>> > >>> diff --git a/lib/crypto/pkcs7_parser.h b/lib/crypto/pkcs7_parser.h > >>> index 6565fdc2d4ca..b8234da45a6a 100644 > >>> --- a/lib/crypto/pkcs7_parser.h > >>> +++ b/lib/crypto/pkcs7_parser.h > >>> @@ -5,6 +5,9 @@ > >>> * Written by David Howells (dhowe...@redhat.com) > >>> */ > >>> > >>> +#ifndef _PKCS7_PARSER_H > >>> +#define _PKCS7_PARSER_H > >>> + > >>> #include <linux/oid_registry.h> > >>> #include <crypto/pkcs7.h> > >>> #include "x509_parser.h" > >>> @@ -63,3 +66,4 @@ struct pkcs7_message { > >>> size_t data_hdrlen; /* Length of Data ASN.1 header */ > >>> const void *data; /* Content Data (or 0) */ > >>> }; > >>> +#endif /* _PKCS7_PARSER_H */ > >>> diff --git a/lib/crypto/x509_parser.h b/lib/crypto/x509_parser.h > >>> index c233f136fb35..4cbdc1d6612d 100644 > >>> --- a/lib/crypto/x509_parser.h > >>> +++ b/lib/crypto/x509_parser.h > >>> @@ -5,6 +5,9 @@ > >>> * Written by David Howells (dhowe...@redhat.com) > >>> */ > >>> > >>> +#ifndef _X509_PARSER_H > >>> +#define _X509_PARSER_H > >>> + > >>> #include <linux/time.h> > >>> #include <crypto/public_key.h> > >>> #include <keys/asymmetric-type.h> > >>> @@ -55,3 +58,4 @@ extern int x509_decode_time(time64_t *_t, size_t > >>> hdrlen, > >>> */ > >>> extern int x509_get_sig_params(struct x509_certificate *cert); > >>> extern int x509_check_for_self_signed(struct x509_certificate *cert); > >>> +#endif /* _X509_PARSER_H */ > >>> diff --git a/lib/efi_loader/efi_image_loader.c > >>> b/lib/efi_loader/efi_image_loader.c > >>> index 6c270ce94f44..899adf8493d0 100644 > >>> --- a/lib/efi_loader/efi_image_loader.c > >>> +++ b/lib/efi_loader/efi_image_loader.c > >>> @@ -14,6 +14,7 @@ > >>> #include <pe.h> > >>> #include <sort.h> > >>> #include "../lib/crypto/pkcs7_parser.h" > >>> +#include "../lib/crypto/x509_parser.h" > >> > >> As pkcs7_parser.h includes x509_parser.h there is no reason to include > >> it again. > > > > No, I don't think so. > > > > My style of coding is that, if a symbol is used in a C file and it is > > defined in a header file, the header file should be *directly* included > > in the C file. > > I believe that it is the common way so that we can avoid any "implicit" > > dependencies among header files. > > We want U-Boot to build as fast as possible. So we should not include > anything twice. When looking through the rest of the U-Boot code you > will have noticed that we do not include malloc.h and stdio.h everywhere > because it is included via common.h.
The same comment here. -Takahiro Akashi > As an example of a patch removing other unnecessary includes see for > instance: > > cmd: fat: remove unused includes > fd0e30b43b6b2401e68dc32c357869c617d4fdd1 > > Best regards > > Heinrich > > > > > Thanks, > > -Takahiro Akashi > > > > > >>> > >>> const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > >>> const efi_guid_t efi_guid_device_path = EFI_DEVICE_PATH_PROTOCOL_GUID; > >>> diff --git a/lib/efi_loader/efi_signature.c > >>> b/lib/efi_loader/efi_signature.c > >>> index 658e3547da37..6ad09e4acbd7 100644 > >>> --- a/lib/efi_loader/efi_signature.c > >>> +++ b/lib/efi_loader/efi_signature.c > >>> @@ -15,6 +15,7 @@ > >>> #include <u-boot/rsa.h> > >>> #include <u-boot/sha256.h> > >>> #include "../lib/crypto/pkcs7_parser.h" > >>> +#include "../lib/crypto/x509_parser.h" > >> > >> Same here. > >> > >> The rest is correct and should be merged. > >> > >> Best regards > >> > >> Heinrich > >> > >>> > >>> const efi_guid_t efi_guid_image_security_database = > >>> EFI_IMAGE_SECURITY_DATABASE_GUID; > >>> > >> >