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? Thanks, Xiaoyu -----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 (#40593): https://edk2.groups.io/g/devel/message/40593 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] -=-=-=-=-=-=-=-=-=-=-=-