[PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Xiaoyu Lu]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1881

Commit(0a1b13fd4d2210e2c3) fix VS2017 build failure
remove useless file in OpensslLib[Crypto].inf,
but we use process_files.py to generate files.
So exclude err_all.c file in process_files.py

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
---
 CryptoPkg/Library/OpensslLib/process_files.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl b/CryptoPkg/Library/OpensslLib/process_files.pl
index e277108f0734..2783ff54f95a 100755
--- a/CryptoPkg/Library/OpensslLib/process_files.pl
+++ b/CryptoPkg/Library/OpensslLib/process_files.pl
@@ -132,6 +132,7 @@ foreach my $product ((@{$unified_info{libraries}},
             # So it can reduce porting time, compile time, library size.
             next if $s =~ "crypto/rand/randfile.c";
             next if $s =~ "crypto/store/";
+            next if $s =~ "crypto/err/err_all.c";
 
             if ($product =~ "libssl") {
                 push @sslfilelist, '  $(OPENSSL_PATH)/' . $s . "\r\n";
-- 
2.7.4

Re: [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Laszlo Ersek]

On 06/19/19 09:19, Xiaoyu Lu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1881
> 
> Commit(0a1b13fd4d2210e2c3) fix VS2017 build failure
> remove useless file in OpensslLib[Crypto].inf,
> but we use process_files.py to generate files.
> So exclude err_all.c file in process_files.py
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> ---
>  CryptoPkg/Library/OpensslLib/process_files.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl b/CryptoPkg/Library/OpensslLib/process_files.pl
> index e277108f0734..2783ff54f95a 100755
> --- a/CryptoPkg/Library/OpensslLib/process_files.pl
> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl
> @@ -132,6 +132,7 @@ foreach my $product ((@{$unified_info{libraries}},
>              # So it can reduce porting time, compile time, library size.
>              next if $s =~ "crypto/rand/randfile.c";
>              next if $s =~ "crypto/store/";
> +            next if $s =~ "crypto/err/err_all.c";
>  
>              if ($product =~ "libssl") {
>                  push @sslfilelist, '  $(OPENSSL_PATH)/' . $s . "\r\n";
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Wang, Jian J]

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Xiaoyu Lu
> Sent: Wednesday, June 19, 2019 3:19 PM
> To: devel@edk2.groups.io
> Cc: Lu, XiaoyuX <xiaoyux.lu@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in
> process_files.py
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1881
> 
> Commit(0a1b13fd4d2210e2c3) fix VS2017 build failure
> remove useless file in OpensslLib[Crypto].inf,
> but we use process_files.py to generate files.
> So exclude err_all.c file in process_files.py
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> ---
>  CryptoPkg/Library/OpensslLib/process_files.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl
> b/CryptoPkg/Library/OpensslLib/process_files.pl
> index e277108f0734..2783ff54f95a 100755
> --- a/CryptoPkg/Library/OpensslLib/process_files.pl
> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl
> @@ -132,6 +132,7 @@ foreach my $product ((@{$unified_info{libraries}},
>              # So it can reduce porting time, compile time, library size.
>              next if $s =~ "crypto/rand/randfile.c";
>              next if $s =~ "crypto/store/";
> +            next if $s =~ "crypto/err/err_all.c";
> 
>              if ($product =~ "libssl") {
>                  push @sslfilelist, '  $(OPENSSL_PATH)/' . $s . "\r\n";
> --
> 2.7.4
> 
> 
> 

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

On Wed, 2019-06-19 at 03:19 -0400, Xiaoyu Lu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1881
> 
> Commit(0a1b13fd4d2210e2c3) fix VS2017 build failure
> remove useless file in OpensslLib[Crypto].inf,
> but we use process_files.py to generate files.
> So exclude err_all.c file in process_files.py
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
> ---
>  CryptoPkg/Library/OpensslLib/process_files.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl b/CryptoPkg/Library/OpensslLib/process_files.pl
> index e277108f0734..2783ff54f95a 100755
> --- a/CryptoPkg/Library/OpensslLib/process_files.pl
> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl
> @@ -132,6 +132,7 @@ foreach my $product ((@{$unified_info{libraries}},
>              # So it can reduce porting time, compile time, library size.
>              next if $s =~ "crypto/rand/randfile.c";
>              next if $s =~ "crypto/store/";
> +            next if $s =~ "crypto/err/err_all.c";
>  
>              if ($product =~ "libssl") {
>                  push @sslfilelist, '  $(OPENSSL_PATH)/' . $s . "\r\n";
> -- 

Hm, this looks like the wrong approach to me. I only ever meant the
exclusions here to be a hack, to fix up things we couldn't properly do
in OpenSSL by disabling features.

Now if you'd fixed up the crypto/store/ exclusion properly by prodding
Richard to add an OPENSSL_NO_STORE option, then the subsequent
inclusion of ERR_load_OSSL_STORE_strings() in err_all.c wouldn't have
led to further hacks to exclude *that* file...

Please submit a PR to OpenSSL to add 'no-store' if you really don't
want it.



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Laszlo Ersek]

On 06/20/19 09:54, David Woodhouse wrote:
> On Wed, 2019-06-19 at 03:19 -0400, Xiaoyu Lu wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1881
>>
>> Commit(0a1b13fd4d2210e2c3) fix VS2017 build failure
>> remove useless file in OpensslLib[Crypto].inf,
>> but we use process_files.py to generate files.
>> So exclude err_all.c file in process_files.py
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Ting Ye <ting.ye@intel.com>
>> Signed-off-by: Xiaoyu Lu <xiaoyux.lu@intel.com>
>> ---
>>  CryptoPkg/Library/OpensslLib/process_files.pl | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/CryptoPkg/Library/OpensslLib/process_files.pl b/CryptoPkg/Library/OpensslLib/process_files.pl
>> index e277108f0734..2783ff54f95a 100755
>> --- a/CryptoPkg/Library/OpensslLib/process_files.pl
>> +++ b/CryptoPkg/Library/OpensslLib/process_files.pl
>> @@ -132,6 +132,7 @@ foreach my $product ((@{$unified_info{libraries}},
>>              # So it can reduce porting time, compile time, library size.
>>              next if $s =~ "crypto/rand/randfile.c";
>>              next if $s =~ "crypto/store/";
>> +            next if $s =~ "crypto/err/err_all.c";
>>  
>>              if ($product =~ "libssl") {
>>                  push @sslfilelist, '  $(OPENSSL_PATH)/' . $s . "\r\n";
>> -- 
> 
> Hm, this looks like the wrong approach to me. I only ever meant the
> exclusions here to be a hack, to fix up things we couldn't properly do
> in OpenSSL by disabling features.
> 
> Now if you'd fixed up the crypto/store/ exclusion properly by prodding
> Richard to add an OPENSSL_NO_STORE option, then the subsequent
> inclusion of ERR_load_OSSL_STORE_strings() in err_all.c wouldn't have
> led to further hacks to exclude *that* file...
> 
> Please submit a PR to OpenSSL to add 'no-store' if you really don't
> want it.

I actually agree about "no-store"; please see point (1) in my earlier
review here:

http://mid.mail-archive.com/0c5b5e95-cb2c-75af-a30b-015dac14b91c@redhat.com

But I've run out of steam on this -- especially the last weeks have seen
me struggle with just reading my email. :(

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

On Thu, 2019-06-20 at 16:46 +0200, Laszlo Ersek wrote:
> > Please submit a PR to OpenSSL to add 'no-store' if you really don't
> > want it.
> 
> I actually agree about "no-store"; please see point (1) in my earlier
> review here:
> 
> http://mid.mail-archive.com/0c5b5e95-cb2c-75af-a30b-015dac14b91c@redhat.com

Hm, you told them to use no-store, and I think you were right. They
seem to have refused purely because of the piffling detail that it
didn't actually exist. I find this suboptimal. Here:

https://github.com/openssl/openssl/pull/9206


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Wang, Jian J]

Hi David,


> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of David
> Woodhouse
> Sent: Friday, June 21, 2019 6:34 AM
> To: devel@edk2.groups.io; lersek@redhat.com; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>;
> Richard Levitte <levitte@openssl.org>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude
> err_all.c in process_files.py
> 
> On Thu, 2019-06-20 at 16:46 +0200, Laszlo Ersek wrote:
> > > Please submit a PR to OpenSSL to add 'no-store' if you really don't
> > > want it.
> >
> > I actually agree about "no-store"; please see point (1) in my earlier
> > review here:
> >
> > http://mid.mail-archive.com/0c5b5e95-cb2c-75af-a30b-
> 015dac14b91c@redhat.com
> 
> Hm, you told them to use no-store, and I think you were right. They
> seem to have refused purely because of the piffling detail that it
> didn't actually exist. I find this suboptimal. Here:
> 
> https://github.com/openssl/openssl/pull/9206
> 

Thanks for the PR. And I agree adding the 'no-store' is the right way to fix
this issue. But the problem here is that we fixated the openssl to one
release tag. We don't change it until we upgrade it to a newer release.
That means any fixes in openssl trunk cannot be used by edk2 immediately,
not to mention there's possibility that the PR will be rejected. So there's
always a lag (maybe a quarter or half year, at least) here.

We have also product release pressure which cannot afford quarters of
waiting for such kind fixes in upstream.

My personal opinion is that, we fix any issue, if we can, in edk2 immediately
for current version of openssl (as workaround), and try to fix it in upstream
for future release at the same time. Once upstream has fixed the issue and
edk2 has decided to upgrade to it, we drop the workaround in edk2. We can
file BZ to track such kind of works.

For this patch, I suggest we still push it. We can drop it and use real fix once
we decide to upgrade openssl future release including your PR.

Thanks,
Jian

> 
> 

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Laszlo Ersek]

On 06/21/19 10:37, Wang, Jian J wrote:
> Hi David,
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of David
>> Woodhouse
>> Sent: Friday, June 21, 2019 6:34 AM
>> To: devel@edk2.groups.io; lersek@redhat.com; Lu, XiaoyuX
>> <xiaoyux.lu@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>;
>> Richard Levitte <levitte@openssl.org>
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude
>> err_all.c in process_files.py
>>
>> On Thu, 2019-06-20 at 16:46 +0200, Laszlo Ersek wrote:
>>>> Please submit a PR to OpenSSL to add 'no-store' if you really don't
>>>> want it.
>>>
>>> I actually agree about "no-store"; please see point (1) in my earlier
>>> review here:
>>>
>>> http://mid.mail-archive.com/0c5b5e95-cb2c-75af-a30b-
>> 015dac14b91c@redhat.com
>>
>> Hm, you told them to use no-store, and I think you were right. They
>> seem to have refused purely because of the piffling detail that it
>> didn't actually exist. I find this suboptimal. Here:
>>
>> https://github.com/openssl/openssl/pull/9206
>>
> 
> Thanks for the PR.

+1

> And I agree adding the 'no-store' is the right way to fix
> this issue. But the problem here is that we fixated the openssl to one
> release tag. We don't change it until we upgrade it to a newer release.
> That means any fixes in openssl trunk cannot be used by edk2 immediately,
> not to mention there's possibility that the PR will be rejected. So there's
> always a lag (maybe a quarter or half year, at least) here.
> 
> We have also product release pressure which cannot afford quarters of
> waiting for such kind fixes in upstream.
> 
> My personal opinion is that, we fix any issue, if we can, in edk2 immediately
> for current version of openssl (as workaround), and try to fix it in upstream
> for future release at the same time. Once upstream has fixed the issue and
> edk2 has decided to upgrade to it, we drop the workaround in edk2. We can
> file BZ to track such kind of works.
> 
> For this patch, I suggest we still push it. We can drop it and use real fix once
> we decide to upgrade openssl future release including your PR.

Right, in the most recent particular case, the time pressure to get
stuff into usable-at-all state, for edk2-stable201905, was huge. I agree
that "reminder BZs" (about backing out temporary downstream fixes) is
the way to go. Example:

https://bugzilla.tianocore.org/show_bug.cgi?id=1897

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Wang, Jian J]

Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, June 25, 2019 3:54 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io;
> dwmw2@infradead.org; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Cc: Ye, Ting <ting.ye@intel.com>; Richard Levitte <levitte@openssl.org>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude
> err_all.c in process_files.py
> 
> On 06/21/19 10:37, Wang, Jian J wrote:
> > Hi David,
> >
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> David
> >> Woodhouse
> >> Sent: Friday, June 21, 2019 6:34 AM
> >> To: devel@edk2.groups.io; lersek@redhat.com; Lu, XiaoyuX
> >> <xiaoyux.lu@intel.com>
> >> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ye, Ting <ting.ye@intel.com>;
> >> Richard Levitte <levitte@openssl.org>
> >> Subject: Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude
> >> err_all.c in process_files.py
> >>
> >> On Thu, 2019-06-20 at 16:46 +0200, Laszlo Ersek wrote:
> >>>> Please submit a PR to OpenSSL to add 'no-store' if you really don't
> >>>> want it.
> >>>
> >>> I actually agree about "no-store"; please see point (1) in my earlier
> >>> review here:
> >>>
> >>> http://mid.mail-archive.com/0c5b5e95-cb2c-75af-a30b-
> >> 015dac14b91c@redhat.com
> >>
> >> Hm, you told them to use no-store, and I think you were right. They
> >> seem to have refused purely because of the piffling detail that it
> >> didn't actually exist. I find this suboptimal. Here:
> >>
> >> https://github.com/openssl/openssl/pull/9206
> >>
> >
> > Thanks for the PR.
> 
> +1
> 
> > And I agree adding the 'no-store' is the right way to fix
> > this issue. But the problem here is that we fixated the openssl to one
> > release tag. We don't change it until we upgrade it to a newer release.
> > That means any fixes in openssl trunk cannot be used by edk2 immediately,
> > not to mention there's possibility that the PR will be rejected. So there's
> > always a lag (maybe a quarter or half year, at least) here.
> >
> > We have also product release pressure which cannot afford quarters of
> > waiting for such kind fixes in upstream.
> >
> > My personal opinion is that, we fix any issue, if we can, in edk2 immediately
> > for current version of openssl (as workaround), and try to fix it in upstream
> > for future release at the same time. Once upstream has fixed the issue and
> > edk2 has decided to upgrade to it, we drop the workaround in edk2. We can
> > file BZ to track such kind of works.
> >
> > For this patch, I suggest we still push it. We can drop it and use real fix once
> > we decide to upgrade openssl future release including your PR.
> 
> Right, in the most recent particular case, the time pressure to get
> stuff into usable-at-all state, for edk2-stable201905, was huge. I agree
> that "reminder BZs" (about backing out temporary downstream fixes) is
> the way to go. 

I take this as agreement. I pushed this patch at (fixed file ext)

51f7a3e6c5192d3f9a0fa63b0b5617c151180ad7

> Example:
> https://bugzilla.tianocore.org/show_bug.cgi?id=1897

Above one is in our plan. I added BZ#1936 for this one.

https://bugzilla.tianocore.org/show_bug.cgi?id=1936

Thanks,
Jian

> 
> Thanks
> Laszlo
> 
> 

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Laszlo Ersek]

Jian,

On 06/25/19 10:58, Wang, Jian J wrote:

> I take this as agreement. I pushed this patch at (fixed file ext)
> 
> 51f7a3e6c5192d3f9a0fa63b0b5617c151180ad7

Please pay more attention to the process.

First, you pushed the patch, but the BZ (1881) is still open. You or
XiaoyuX should have closed the BZ with a reference to the commit. I'm
doing that now.

Second, in commit 51f7a3e6c519, you failed to pick up my R-b from the list:

http://mid.mail-archive.com/22434e9b-745c-671c-2b71-43f5ecb49848@redhat.com

My suggestion is, just before you push a patch, go through the entire
mailing list thread for one last time, to make sure no feedback is lost.

(Obviously, if your MUA doesn't offer a threaded view, this is quite
difficult. Even in that case though, you could filter the list folder
for the particular subject, at the least.)

> Above one is in our plan. I added BZ#1936 for this one.
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1936

That's appreciated.

Laszlo

Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude err_all.c in process_files.py

[Wang, Jian J]

Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, June 26, 2019 8:46 PM
> To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>;
> dwmw2@infradead.org; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Cc: Ye, Ting <ting.ye@intel.com>; Richard Levitte <levitte@openssl.org>
> Subject: Re: [edk2-devel] [PATCH v1 1/1] CryptoPkg/OpensslLib: Exclude
> err_all.c in process_files.py
> 
> Jian,
> 
> On 06/25/19 10:58, Wang, Jian J wrote:
> 
> > I take this as agreement. I pushed this patch at (fixed file ext)
> >
> > 51f7a3e6c5192d3f9a0fa63b0b5617c151180ad7
> 
> Please pay more attention to the process.
> 
> First, you pushed the patch, but the BZ (1881) is still open. You or
> XiaoyuX should have closed the BZ with a reference to the commit. I'm
> doing that now.
> 

Got it. I'll keep it in mind. Thanks.

> Second, in commit 51f7a3e6c519, you failed to pick up my R-b from the list:
> 
> http://mid.mail-archive.com/22434e9b-745c-671c-2b71-
> 43f5ecb49848@redhat.com
> 
> My suggestion is, just before you push a patch, go through the entire
> mailing list thread for one last time, to make sure no feedback is lost.
> 
> (Obviously, if your MUA doesn't offer a threaded view, this is quite
> difficult. Even in that case though, you could filter the list folder
> for the particular subject, at the least.)
> 

My apologies. I remember I did go through the list but still missed it.

Regards,
Jian
> > Above one is in our plan. I added BZ#1936 for this one.
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1936
> 
> That's appreciated.
> 
> Laszlo
> 
>