On 05/14/19 14:41, Lu, XiaoyuX wrote: > Hi Laszlo, > I think process_files.pl is used to control which of OpenSSL source > files we need which we don't need. > If we have unwanted files, the effective way is exclude them directly > in process_files.pl. > You can see process_files.pl > > > 129 ┆ ┆ ┆ ┆ ┆ next if $s =~ "crypto/bio/b_print.c"; > > Qing Long also use this way to exclude unwanted file. > > If the file (example: rand_unix.c) is used by OpenSSL internal, We > can't exclude it in process_files.pl, > Than we consider submitting patches for OpenSSL. > > What do you think?
I agree to excluding "rand_unix.c" similarly to "b_print.c"; that is, with "next if" in the "process_files.pl" script. Thanks Laszlo > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo > Ersek > Sent: Monday, May 13, 2019 11:13 PM > To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Ye, Ting <ting...@intel.com> > Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude > unnecessary files in process_files.pl > > On 05/10/19 10:51, Xiaoyu lu wrote: >> Hi, Laszlo: >> >> Thank you for your time. >> >> I try the method you mentioned. >> >>> (1) Therefore, the right thing to do here is to add "no-store" to the above >>> list, in my opinion. Can you try that, please? >>> >>> And, this change should be a standalone patch, similarly to patch v2 1/6 in >>> this series. >> >> (1) OpenSSL configure script don't support no-store option. >> It will lead to configure error. >> >> Unsupported options: no-store >> >>> (2a) Therefore, we should modify the "randfile.c" source file, with an >>> upstream OpenSSL contribution, to hide the function definitions, when >>> OPENSSL_SYS_UEFI is defined. In other words, continue with Qin Long's >>> approach from commit fb4844bbc62f. >> >> I think this is the best way. But the openssl community takes time to accept >> the patch. >> I just let OpenSSL work for UEFI. So UEFI can use the new algorithm in >> OpenSSL_1_1_1. >> I am willing to continue to modify this later. > > Please pick one of two: > > - file a new TianoCore BZ about cleaning up this technical debt, and paste > the BZ URL into the code, as a comment > > - delay TianoCore BZ#1089 to the next edk2-stable release, and work with > upstream OpenSSL to compile out parts of "randfile.c". > > Thanks > Laszlo > >> >>> (2b) Alternatively, I'm noticing that "rand" is just another module >>> (similar to "store", see above). Assuming we really don't need RAND_* >>> functions for anything in edk2: have we tried configuring OpenSSL, for the >>> edk2 build, with the "no-rand" parameter? >> >> (2) I'm afraid not. Same as (1) >> >> ***** Unsupported options: no-rand >> >> Thanks, >> Xiaoyu. >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Thursday, May 9, 2019 9:43 PM >> To: devel@edk2.groups.io; Lu, XiaoyuX <xiaoyux...@intel.com> >> Cc: Wang, Jian J <jian.j.w...@intel.com>; Ye, Ting <ting...@intel.com> >> Subject: Re: [edk2-devel] [PATCH v2 2/6] CryptoPkg/OpensslLib: Exclude >> unnecessary files in process_files.pl >> >> On 05/09/19 07:23, Xiaoyu lu wrote: >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1089 >>> >>> When running process_files.py to configure OpenSSL, we can exclude >>> some unnecessary files. This can reduce porting time, compiling time >>> and library size. >> >> Indeed. >> >>> OpenSSL_1_1_1(1708e3e85b4a8) add a STORE module (crypto/store/*). >> >> This statement is incorrect (or, minimally, inexact). According to the >> following command: >> >> $ git log --oneline --reverse OpenSSL_1_1_1b -- crypto/store/ \ >> | head -n 1 >> >> the first OpenSSL commit that added files to crypto/store/ was: >> >>> commit a5db6fa5760f21d16d59e025e930c02456e00fef >>> Author: Richard Levitte <levi...@openssl.org> >>> Date: Thu May 1 03:53:12 2003 +0000 >>> >>> Define a STORE type. For documentation, read the entry in CHANGES, >>> crypto/store/README, crypto/store/store.h and crypto/store/str_locl.h. >> >> This commit goes back to 2003, and is part of releae OpenSSL_0_9_7d. >> >> Instead, let's check what the following command reports: >> >> $ git log --oneline --reverse \ >> OpenSSL_1_1_0j..OpenSSL_1_1_1b -- crypto/store/ \ >> | head -1 >> >> It states that the first commit after OpenSSL_1_1_0j, but not after >> OpenSSL_1_1_1b, to modify the "crypto/store/" subdirectory, was commit >> 71a5516dcc8a ("Add the STORE module", 2017-06-29). >> >> If we investigate that commit: >> >> $ git show --stat 71a5516dcc8a >> >> we see that the commit modifies the Configure script: >> >>> Configure | 2 +- >> >> So let's check that part of the diff in detail: >> >> $ git show 71a5516dcc8a -- Configure >> >> And we get: >> >>> diff --git a/Configure b/Configure >>> index 2eacb2312e34..e302a58abb71 100755 >>> --- a/Configure >>> +++ b/Configure >>> @@ -310,7 +310,7 @@ $config{sdirs} = [ >>> "bn", "ec", "rsa", "dsa", "dh", "dso", "engine", >>> "buffer", "bio", "stack", "lhash", "rand", "err", >>> "evp", "asn1", "pem", "x509", "x509v3", "conf", "txt_db", "pkcs7", >>> "pkcs12", "comp", "ocsp", "ui", >>> - "cms", "ts", "srp", "cmac", "ct", "async", "kdf" >>> + "cms", "ts", "srp", "cmac", "ct", "async", "kdf", "store" >>> ]; >>> # test/ subdirectories to build >>> $config{tdirs} = [ "ossl_shim" ]; >> >> We can see that the "store" module is added after modules such as "cms", >> "ts", "srp", and so on. >> >> Now, if you look at "CryptoPkg/Library/OpensslLib/process_files.pl", you >> find (with edk2 master being at commit 49693202ec9c): >> >> 49 "./Configure", >> 50 "UEFI", >> 51 "no-afalgeng", >> 52 "no-asm", >> 53 "no-async", <---- disables "async" >> 54 "no-autoalginit", >> 55 "no-autoerrinit", >> 56 "no-bf", >> 57 "no-blake2", >> 58 "no-camellia", >> 59 "no-capieng", >> 60 "no-cast", >> 61 "no-chacha", >> 62 "no-cms", <---- disables "cms" >> 63 "no-ct", <---- disables "ct" >> 64 "no-deprecated", >> 65 "no-dgram", >> 66 "no-dsa", >> 67 "no-dynamic-engine", >> 68 "no-ec", >> 69 "no-ec2m", >> 70 "no-engine", >> 71 "no-err", >> 72 "no-filenames", >> 73 "no-gost", >> 74 "no-hw", >> 75 "no-idea", >> 76 "no-mdc2", >> 77 "no-pic", >> 78 "no-ocb", >> 79 "no-poly1305", >> 80 "no-posix-io", >> 81 "no-rc2", >> 82 "no-rfc3779", >> 83 "no-rmd160", >> 84 "no-scrypt", >> 85 "no-seed", >> 86 "no-sock", >> 87 "no-srp", <---- disables "srp" >> 88 "no-ssl", >> 89 "no-stdio", >> 90 "no-threads", >> 91 "no-ts", <---- disables "ts" >> 92 "no-ui", >> 93 "no-whirlpool" >> >> (1) Therefore, the right thing to do here is to add "no-store" to the above >> list, in my opinion. Can you try that, please? >> >> And, this change should be a standalone patch, similarly to patch v2 1/6 in >> this series. >> >>> But UEFI don't use them. So exclude these files. >> >>> This file, crypto/rand/randfile.c, have been modified between >>> OpenSSL_1_1_0j(74f2d9c1ec5f5) and OpenSSL_1_1_1b(50eaac9f33376672). >>> It requires more crt runtime support. But UEFI don't use it. >>> So exclude the file. >> >> I think I disagree with this approach. >> >> In OpenSSL commit fb4844bbc62f -- "Add UEFI flag for rand build", >> 2015-09-03, part of OpenSSL_1_1_0 --, Qin Long customized >> "crypto/rand/rand_unix.c". So that, when OPENSSL_SYS_UEFI was >> #defined, the real RAND_poll() function was replaced by a stub that >> would always report failure. (So this was a safe stub.) >> >> In OpenSSL commit 8389ec4b4950 -- "Add --with-rand-seed", 2017-07-22 --, the >> feature test itself has been reworked (see the previous patch in this >> series). However, it remains the case that "rand_unix.c" consumes and honors >> the OPENSSL_SYS_UEFI macro. >> >> So, let's check the "randfile.c" file. It defines three functions: >> - RAND_load_file >> - RAND_write_file >> - RAND_file_name >> >> Nothing inside the OpenSSL library calls them (they exist purely for client >> code), and nothing in edk2 calls them either. >> >> (2a) Therefore, we should modify the "randfile.c" source file, with an >> upstream OpenSSL contribution, to hide the function definitions, when >> OPENSSL_SYS_UEFI is defined. In other words, continue with Qin Long's >> approach from commit fb4844bbc62f. >> >> (2b) Alternatively, I'm noticing that "rand" is just another module (similar >> to "store", see above). Assuming we really don't need RAND_* functions for >> anything in edk2: have we tried configuring OpenSSL, for the edk2 build, >> with the "no-rand" parameter? >> >> Thanks, >> Laszlo >> >>> >>> Cc: Jian J Wang <jian.j.w...@intel.com> >>> Cc: Ting Ye <ting...@intel.com> >>> Signed-off-by: Xiaoyu Lu <xiaoyux...@intel.com> >>> --- >>> CryptoPkg/Library/OpensslLib/process_files.pl | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl >>> b/CryptoPkg/Library/OpensslLib/process_files.pl >>> index 6c136cc..e277108 100755 >>> --- a/CryptoPkg/Library/OpensslLib/process_files.pl >>> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl >>> @@ -127,6 +127,12 @@ foreach my $product ((@{$unified_info{libraries}}, >>> foreach my $s (@{$unified_info{sources}->{$o}}) { >>> next if ($unified_info{generate}->{$s}); >>> next if $s =~ "crypto/bio/b_print.c"; >>> + >>> + # No need to add unused files in UEFI. >>> + # So it can reduce porting time, compile time, library size. >>> + next if $s =~ "crypto/rand/randfile.c"; >>> + next if $s =~ "crypto/store/"; >>> + >>> if ($product =~ "libssl") { >>> push @sslfilelist, ' $(OPENSSL_PATH)/' . $s . "\r\n"; >>> next; >>> >> >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#40608): https://edk2.groups.io/g/devel/message/40608 Mute This Topic: https://groups.io/mt/31552209/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
