[PATCH] lib/crypto, efi_loader: move some headers to include/crypto

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Thu, Apr 23, 2020 at 09:33:37AM +0200, Heinrich Schuchardt wrote:
> On 23.04.20 02:31, AKASHI Takahiro wrote:
> > Heinrich,
> >
> > On Tue, Apr 21, 2020 at 12:26:08PM +0200, Heinrich Schuchardt wrote:
> >> On 4/21/20 2:38 AM, AKASHI Takahiro wrote:
> >>> Pkcs7_parse.h and x509_parser.h are used in UEFI subsystem, in particular,
> >>> secure boot. So move them to include/crypto to avoid relative paths.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>> ---
> >>>  {lib => include}/crypto/pkcs7_parser.h | 0
> >>>  {lib => include}/crypto/x509_parser.h  | 0
> >>>  lib/crypto/pkcs7_parser.c              | 4 ++++
> >>>  lib/crypto/x509_cert_parser.c          | 4 ++++
> >>>  lib/crypto/x509_public_key.c           | 6 ++++--
> >>>  lib/efi_loader/efi_image_loader.c      | 4 ++--
> >>>  lib/efi_loader/efi_signature.c         | 4 ++--
> >>>  lib/efi_loader/efi_variable.c          | 2 +-
> >>>  test/lib/asn1.c                        | 4 ++--
> >>>  9 files changed, 19 insertions(+), 9 deletions(-)
> >>>  rename {lib => include}/crypto/pkcs7_parser.h (100%)
> >>>  rename {lib => include}/crypto/x509_parser.h (100%)
> >>>
> >>> diff --git a/lib/crypto/pkcs7_parser.h b/include/crypto/pkcs7_parser.h
> >>> similarity index 100%
> >>> rename from lib/crypto/pkcs7_parser.h
> >>> rename to include/crypto/pkcs7_parser.h
> >>> diff --git a/lib/crypto/x509_parser.h b/include/crypto/x509_parser.h
> >>> similarity index 100%
> >>> rename from lib/crypto/x509_parser.h
> >>> rename to include/crypto/x509_parser.h
> >>> diff --git a/lib/crypto/pkcs7_parser.c b/lib/crypto/pkcs7_parser.c
> >>> index f5dda1179f8a..0ee207b6b1c8 100644
> >>> --- a/lib/crypto/pkcs7_parser.c
> >>> +++ b/lib/crypto/pkcs7_parser.c
> >>> @@ -20,7 +20,11 @@
> >>>  #include <linux/err.h>
> >>>  #include <linux/oid_registry.h>
> >>>  #include <crypto/public_key.h>
> >>> +#ifdef __UBOOT__
> >>> +#include <crypto/pkcs7_parser.h>
> >>> +#else
> >>>  #include "pkcs7_parser.h"
> >>> +#endif
> >>>  #include "pkcs7.asn1.h"
> >>>
> >>>  MODULE_DESCRIPTION("PKCS#7 parser");
> >>> diff --git a/lib/crypto/x509_cert_parser.c b/lib/crypto/x509_cert_parser.c
> >>> index 4e41cffd2301..18f5407a076c 100644
> >>> --- a/lib/crypto/x509_cert_parser.c
> >>> +++ b/lib/crypto/x509_cert_parser.c
> >>> @@ -18,7 +18,11 @@
> >>>  #include <linux/string.h>
> >>>  #endif
> >>>  #include <crypto/public_key.h>
> >>> +#ifdef __UBOOT__
> >>> +#include <crypto/x509_parser.h>
> >>> +#else
> >>>  #include "x509_parser.h"
> >>> +#endif
> >>>  #include "x509.asn1.h"
> >>>  #include "x509_akid.asn1.h"
> >>>
> >>> diff --git a/lib/crypto/x509_public_key.c b/lib/crypto/x509_public_key.c
> >>> index 676c0df17410..571af9a0adf9 100644
> >>> --- a/lib/crypto/x509_public_key.c
> >>> +++ b/lib/crypto/x509_public_key.c
> >>> @@ -16,15 +16,17 @@
> >>>  #include <linux/module.h>
> >>>  #endif
> >>>  #include <linux/kernel.h>
> >>> -#ifndef __UBOOT__
> >>> +#ifdef __UBOOT__
> >>> +#include <crypto/x509_parser.h>
> >>> +#else
> >>>  #include <linux/slab.h>
> >>>  #include <keys/asymmetric-subtype.h>
> >>>  #include <keys/asymmetric-parser.h>
> >>>  #include <keys/system_keyring.h>
> >>>  #include <crypto/hash.h>
> >>>  #include "asymmetric_keys.h"
> >>> -#endif
> >>>  #include "x509_parser.h"
> >>> +#endif
> >>>
> >>>  /*
> >>>   * Set up the signature parameters in an X.509 certificate.  This involves
> >>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> >>> index 899adf8493d0..f59b9a01140e 100644
> >>> --- a/lib/efi_loader/efi_image_loader.c
> >>> +++ b/lib/efi_loader/efi_image_loader.c
> >>> @@ -13,8 +13,8 @@
> >>>  #include <malloc.h>
> >>>  #include <pe.h>
> >>>  #include <sort.h>
> >>> -#include "../lib/crypto/pkcs7_parser.h"
> >>> -#include "../lib/crypto/x509_parser.h"
> >>> +#include "crypto/pkcs7_parser.h"
> >>> +#include "crypto/x509_parser.h"
> >>
> >> Thanks for fixing this.
> >>
> >> x509_parser.h is included in pkcs7_parser.h. Please, remove the
> >> superfluous line.
> >
> > See my comment in [1]
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2020-April/408160.html
> 
> 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.

I can't agree with you.
"common.h" is not a good example; its purpose is to aggregate
some some useful header files so that people don't get bothered
with finding out an exact set of header files needed.

"common.h" is even a *bad* example if you say, "We want U-Boot to build
as fast as possible." because "common.h" may contain a lot of header
files that may never be used in a given C file.

So I won't change my patch.

-Takahiro Akashi

> As an example of a patch removing other unnecessary includes see for
> instance:
> 
> cmd: fat: remove unused includes
> fd0e30b43b6b2401e68dc32c357869c617d4fdd1
> 
> Best regards
> 
> Heinrich
> 
> >
> > -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 6ad09e4acbd7..0ead10203aeb 100644
> >>> --- a/lib/efi_loader/efi_signature.c
> >>> +++ b/lib/efi_loader/efi_signature.c
> >>> @@ -10,12 +10,12 @@
> >>>  #include <image.h>
> >>>  #include <hexdump.h>
> >>>  #include <malloc.h>
> >>> +#include <crypto/pkcs7_parser.h>
> >>> +#include <crypto/x509_parser.h>
> >>
> >> Same here.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>  #include <linux/compat.h>
> >>>  #include <linux/oid_registry.h>
> >>>  #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;
> >>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >>> index 7df881a74b44..0c6d1deb58eb 100644
> >>> --- a/lib/efi_loader/efi_variable.c
> >>> +++ b/lib/efi_loader/efi_variable.c
> >>> @@ -12,9 +12,9 @@
> >>>  #include <malloc.h>
> >>>  #include <rtc.h>
> >>>  #include <search.h>
> >>> +#include <crypto/pkcs7_parser.h>
> >>>  #include <linux/compat.h>
> >>>  #include <u-boot/crc.h>
> >>> -#include "../lib/crypto/pkcs7_parser.h"
> >>>
> >>>  enum efi_secure_mode {
> >>>  	EFI_MODE_SETUP,
> >>> diff --git a/test/lib/asn1.c b/test/lib/asn1.c
> >>> index d2b3f67e68da..8661fdd30687 100644
> >>> --- a/test/lib/asn1.c
> >>> +++ b/test/lib/asn1.c
> >>> @@ -13,10 +13,10 @@
> >>>  #include <test/ut.h>
> >>>
> >>>  #ifdef CONFIG_PKCS7_MESSAGE_PARSER
> >>> -#include "../../lib/crypto/pkcs7_parser.h"
> >>> +#include <crypto/pkcs7_parser.h>
> >>>  #else
> >>>  #ifdef CONFIG_X509_CERTIFICATE_PARSER
> >>> -#include "../../lib/crypto/x509_parser.h"
> >>> +#include <crypto/x509_parser.h>
> >>>  #endif
> >>>  #endif
> >>>
> >>>
> >>
>