| Message ID | 20200421003752.16009-1-takahiro.akashi@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7b8b63fb8ead9bd4041ab5424deb465f14ee172a |
| Headers | show |
| Series | lib/crypto, efi_loader: avoid multiple inclusions of header files | expand |
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.akashi at linaro.org> > Suggested-by: Heinrich Schuchardt <xypron.glpk at 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 (dhowells at 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 (dhowells at 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. > > 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; >
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.akashi at linaro.org> > > Suggested-by: Heinrich Schuchardt <xypron.glpk at 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 (dhowells at 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 (dhowells at 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. 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; > > >
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.akashi at linaro.org> >>> Suggested-by: Heinrich Schuchardt <xypron.glpk at 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 (dhowells at 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 (dhowells at 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. 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; >>> >>
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.akashi at linaro.org> > >>> Suggested-by: Heinrich Schuchardt <xypron.glpk at 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 (dhowells at 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 (dhowells at 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; > >>> > >> >
On 4/24/20 5:50 AM, AKASHI Takahiro wrote: > 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.akashi at linaro.org> >>>>> Suggested-by: Heinrich Schuchardt <xypron.glpk at gmx.de> Patch is merged. Thanks - Heinrich
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 (dhowells at 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 (dhowells at 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" 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" const efi_guid_t efi_guid_image_security_database = EFI_IMAGE_SECURITY_DATABASE_GUID;
By adding extra symbols, we can now avoid including x509_parser and pkcs7_parser.h files multiple times. Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org> Suggested-by: Heinrich Schuchardt <xypron.glpk at 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(+)