* [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
@ 2017-10-20 15:10 Peter Jones
2017-10-20 17:12 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Peter Jones @ 2017-10-20 15:10 UTC (permalink / raw)
To: edk2-devel
clang-analyzer noticed this:
Pk/CryptPkcs7Verify.c:600:5: warning: Value stored to 'OldSize' is never read
OldSize = BufferSize;
^ ~~~~~~~~~~
Pk/CryptPkcs7Verify.c:644:5: warning: Value stored to 'OldSize' is never read
OldSize = BufferSize;
^ ~~~~~~~~~~
2 warnings generated.
These are each immediately followed by a loop that initializes them (to
the same values) a second time, and are otherwise only referenced inside
that loop, so there's just no point to these assignments at all.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Peter Jones <pjones@redhat.com>
---
CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
index d564591cb7f9..bf67a1f569a2 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
@@ -612,7 +612,6 @@ Pkcs7GetCertificatesList (
if (CtxChain != NULL) {
BufferSize = sizeof (UINT8);
- OldSize = BufferSize;
CertBuf = NULL;
for (Index = 0; ; Index++) {
@@ -656,7 +655,6 @@ Pkcs7GetCertificatesList (
if (CtxUntrusted != NULL) {
BufferSize = sizeof (UINT8);
- OldSize = BufferSize;
CertBuf = NULL;
for (Index = 0; ; Index++) {
--
2.14.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
2017-10-20 15:10 [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations Peter Jones
@ 2017-10-20 17:12 ` Laszlo Ersek
2017-10-20 18:21 ` Peter Jones
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-10-20 17:12 UTC (permalink / raw)
To: Peter Jones; +Cc: edk2-devel, Shi, Steven, Qin Long, Ting Ye
(adding CryptoPkg maintainers from Maintainers.txt, plus Steven for
clang)
On 10/20/17 17:10, Peter Jones wrote:
> clang-analyzer noticed this:
>
> Pk/CryptPkcs7Verify.c:600:5: warning: Value stored to 'OldSize' is never read
> OldSize = BufferSize;
> ^ ~~~~~~~~~~
> Pk/CryptPkcs7Verify.c:644:5: warning: Value stored to 'OldSize' is never read
> OldSize = BufferSize;
> ^ ~~~~~~~~~~
> 2 warnings generated.
Interesting; Steven runs clang (incl. clang-analyzer AFAIK) frequently,
and I don't recall an earlier report of this.
Anyway,
>
> These are each immediately followed by a loop that initializes them (to
> the same values) a second time, and are otherwise only referenced inside
> that loop, so there's just no point to these assignments at all.
I agree.
While each of both loops might fail to reach the inner assignment to
OldSize -- in case the first X509PopCertificate() call fails in the
loop, for Index=0 --, OldSize is still never read after each loop
(before another assignment to it is reached). So setting OldSize to
anything at all before the loops is superfluous.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
> CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> index d564591cb7f9..bf67a1f569a2 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> @@ -612,7 +612,6 @@ Pkcs7GetCertificatesList (
>
> if (CtxChain != NULL) {
> BufferSize = sizeof (UINT8);
> - OldSize = BufferSize;
> CertBuf = NULL;
>
> for (Index = 0; ; Index++) {
> @@ -656,7 +655,6 @@ Pkcs7GetCertificatesList (
>
> if (CtxUntrusted != NULL) {
> BufferSize = sizeof (UINT8);
> - OldSize = BufferSize;
> CertBuf = NULL;
>
> for (Index = 0; ; Index++) {
>
For your patch:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Assuming the maintainers are fine with the patch as well, I suggest that
they please replace the word "initializations" with "assignments" in the
subject, to be pedantic on the C-lang level.
(Side note: I would even move OldSize to a lot tighter scope:
> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> index d564591cb7f9..31a9ecd59ff6 100644
> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> @@ -477,7 +477,6 @@ Pkcs7GetCertificatesList (
> UINT8 *CertBuf;
> UINT8 *OldBuf;
> UINTN BufferSize;
> - UINTN OldSize;
> UINT8 *SingleCert;
> UINTN CertSize;
>
> @@ -612,10 +611,11 @@ Pkcs7GetCertificatesList (
>
> if (CtxChain != NULL) {
> BufferSize = sizeof (UINT8);
> - OldSize = BufferSize;
> CertBuf = NULL;
>
> for (Index = 0; ; Index++) {
> + UINTN OldSize;
> +
> Status = X509PopCertificate (CtxChain, &SingleCert, &CertSize);
> if (!Status) {
> break;
> @@ -656,10 +656,11 @@ Pkcs7GetCertificatesList (
>
> if (CtxUntrusted != NULL) {
> BufferSize = sizeof (UINT8);
> - OldSize = BufferSize;
> CertBuf = NULL;
>
> for (Index = 0; ; Index++) {
> + UINTN OldSize;
> +
> Status = X509PopCertificate (CtxUntrusted, &SingleCert, &CertSize);
> if (!Status) {
> break;
However, many edk2 maintainers don't like tight scoping like this.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
2017-10-20 17:12 ` Laszlo Ersek
@ 2017-10-20 18:21 ` Peter Jones
2017-10-23 3:02 ` Long, Qin
0 siblings, 1 reply; 6+ messages in thread
From: Peter Jones @ 2017-10-20 18:21 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel, Shi, Steven, Qin Long, Ting Ye
> Assuming the maintainers are fine with the patch as well, I suggest that
> they please replace the word "initializations" with "assignments" in the
> subject, to be pedantic on the C-lang level.
Well, that's why I said "initializations" instead of "initializers", but if
it's more clear to you, I'm fine with your way.
> (Side note: I would even move OldSize to a lot tighter scope:
>
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> > index d564591cb7f9..31a9ecd59ff6 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> > @@ -477,7 +477,6 @@ Pkcs7GetCertificatesList (
> > UINT8 *CertBuf;
> > UINT8 *OldBuf;
> > UINTN BufferSize;
> > - UINTN OldSize;
> > UINT8 *SingleCert;
> > UINTN CertSize;
> >
> > @@ -612,10 +611,11 @@ Pkcs7GetCertificatesList (
> >
> > if (CtxChain != NULL) {
> > BufferSize = sizeof (UINT8);
> > - OldSize = BufferSize;
> > CertBuf = NULL;
> >
> > for (Index = 0; ; Index++) {
> > + UINTN OldSize;
> > +
> > Status = X509PopCertificate (CtxChain, &SingleCert, &CertSize);
> > if (!Status) {
> > break;
> > @@ -656,10 +656,11 @@ Pkcs7GetCertificatesList (
> >
> > if (CtxUntrusted != NULL) {
> > BufferSize = sizeof (UINT8);
> > - OldSize = BufferSize;
> > CertBuf = NULL;
> >
> > for (Index = 0; ; Index++) {
> > + UINTN OldSize;
> > +
> > Status = X509PopCertificate (CtxUntrusted, &SingleCert, &CertSize);
> > if (!Status) {
> > break;
>
> However, many edk2 maintainers don't like tight scoping like this.)
I had considered this and guessed it was probably against the coding style or
it would have been done this way already. IMO it's better in every way.
--
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
2017-10-20 18:21 ` Peter Jones
@ 2017-10-23 3:02 ` Long, Qin
2017-10-24 7:51 ` Laszlo Ersek
0 siblings, 1 reply; 6+ messages in thread
From: Long, Qin @ 2017-10-23 3:02 UTC (permalink / raw)
To: Peter Jones, Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Shi, Steven, Ye, Ting
This looks good to me.
Reviewed-by: Long Qin qin.long@intel.com<mailto:qin.long@intel.com>
Best Regards & Thanks,
LONG, Qin
From: Peter Jones [mailto:pjones@redhat.com]
Sent: Saturday, October 21, 2017 2:22 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel@lists.01.org; Shi, Steven <steven.shi@intel.com>; Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
> Assuming the maintainers are fine with the patch as well, I suggest that
> they please replace the word "initializations" with "assignments" in the
> subject, to be pedantic on the C-lang level.
Well, that's why I said "initializations" instead of "initializers", but if
it's more clear to you, I'm fine with your way.
> (Side note: I would even move OldSize to a lot tighter scope:
>
> > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> > index d564591cb7f9..31a9ecd59ff6 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
> > @@ -477,7 +477,6 @@ Pkcs7GetCertificatesList (
> > UINT8 *CertBuf;
> > UINT8 *OldBuf;
> > UINTN BufferSize;
> > - UINTN OldSize;
> > UINT8 *SingleCert;
> > UINTN CertSize;
> >
> > @@ -612,10 +611,11 @@ Pkcs7GetCertificatesList (
> >
> > if (CtxChain != NULL) {
> > BufferSize = sizeof (UINT8);
> > - OldSize = BufferSize;
> > CertBuf = NULL;
> >
> > for (Index = 0; ; Index++) {
> > + UINTN OldSize;
> > +
> > Status = X509PopCertificate (CtxChain, &SingleCert, &CertSize);
> > if (!Status) {
> > break;
> > @@ -656,10 +656,11 @@ Pkcs7GetCertificatesList (
> >
> > if (CtxUntrusted != NULL) {
> > BufferSize = sizeof (UINT8);
> > - OldSize = BufferSize;
> > CertBuf = NULL;
> >
> > for (Index = 0; ; Index++) {
> > + UINTN OldSize;
> > +
> > Status = X509PopCertificate (CtxUntrusted, &SingleCert, &CertSize);
> > if (!Status) {
> > break;
>
> However, many edk2 maintainers don't like tight scoping like this.)
I had considered this and guessed it was probably against the coding style or
it would have been done this way already. IMO it's better in every way.
--
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
2017-10-23 3:02 ` Long, Qin
@ 2017-10-24 7:51 ` Laszlo Ersek
2017-10-24 8:15 ` Long, Qin
0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-10-24 7:51 UTC (permalink / raw)
To: Long, Qin, Peter Jones; +Cc: edk2-devel@lists.01.org, Shi, Steven, Ye, Ting
Qin,
On 10/23/17 05:02, Long, Qin wrote:
> This looks good to me.
> Reviewed-by: Long Qin qin.long@intel.com<mailto:qin.long@intel.com>
Do you want me to push the patch, or do you prefer to push it yourself?
Thanks!
Laszlo
> From: Peter Jones [mailto:pjones@redhat.com]
> Sent: Saturday, October 21, 2017 2:22 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org; Shi, Steven <steven.shi@intel.com>; Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
>
>> Assuming the maintainers are fine with the patch as well, I suggest that
>> they please replace the word "initializations" with "assignments" in the
>> subject, to be pedantic on the C-lang level.
>
> Well, that's why I said "initializations" instead of "initializers", but if
> it's more clear to you, I'm fine with your way.
>
>> (Side note: I would even move OldSize to a lot tighter scope:
>>
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> index d564591cb7f9..31a9ecd59ff6 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> @@ -477,7 +477,6 @@ Pkcs7GetCertificatesList (
>>> UINT8 *CertBuf;
>>> UINT8 *OldBuf;
>>> UINTN BufferSize;
>>> - UINTN OldSize;
>>> UINT8 *SingleCert;
>>> UINTN CertSize;
>>>
>>> @@ -612,10 +611,11 @@ Pkcs7GetCertificatesList (
>>>
>>> if (CtxChain != NULL) {
>>> BufferSize = sizeof (UINT8);
>>> - OldSize = BufferSize;
>>> CertBuf = NULL;
>>>
>>> for (Index = 0; ; Index++) {
>>> + UINTN OldSize;
>>> +
>>> Status = X509PopCertificate (CtxChain, &SingleCert, &CertSize);
>>> if (!Status) {
>>> break;
>>> @@ -656,10 +656,11 @@ Pkcs7GetCertificatesList (
>>>
>>> if (CtxUntrusted != NULL) {
>>> BufferSize = sizeof (UINT8);
>>> - OldSize = BufferSize;
>>> CertBuf = NULL;
>>>
>>> for (Index = 0; ; Index++) {
>>> + UINTN OldSize;
>>> +
>>> Status = X509PopCertificate (CtxUntrusted, &SingleCert, &CertSize);
>>> if (!Status) {
>>> break;
>>
>> However, many edk2 maintainers don't like tight scoping like this.)
>
> I had considered this and guessed it was probably against the coding style or
> it would have been done this way already. IMO it's better in every way.
>
> --
> Peter
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
2017-10-24 7:51 ` Laszlo Ersek
@ 2017-10-24 8:15 ` Long, Qin
0 siblings, 0 replies; 6+ messages in thread
From: Long, Qin @ 2017-10-24 8:15 UTC (permalink / raw)
To: Laszlo Ersek, Peter Jones; +Cc: Ye, Ting, edk2-devel@lists.01.org
The patch was already push @b5a985ca9237b551618cd97b1b71af2fff55e209
I forgot to inform that. Thanks, Laszlo.
Best Regards & Thanks,
LONG, Qin
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, October 24, 2017 3:51 PM
To: Long, Qin <qin.long@intel.com>; Peter Jones <pjones@redhat.com>
Cc: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
Qin,
On 10/23/17 05:02, Long, Qin wrote:
> This looks good to me.
> Reviewed-by: Long Qin qin.long@intel.com<mailto:qin.long@intel.com>
Do you want me to push the patch, or do you prefer to push it yourself?
Thanks!
Laszlo
> From: Peter Jones [mailto:pjones@redhat.com]
> Sent: Saturday, October 21, 2017 2:22 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org; Shi, Steven <steven.shi@intel.com>; Long,
> Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
>
>> Assuming the maintainers are fine with the patch as well, I suggest
>> that they please replace the word "initializations" with
>> "assignments" in the subject, to be pedantic on the C-lang level.
>
> Well, that's why I said "initializations" instead of "initializers",
> but if it's more clear to you, I'm fine with your way.
>
>> (Side note: I would even move OldSize to a lot tighter scope:
>>
>>> diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> index d564591cb7f9..31a9ecd59ff6 100644
>>> --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c
>>> @@ -477,7 +477,6 @@ Pkcs7GetCertificatesList (
>>> UINT8 *CertBuf;
>>> UINT8 *OldBuf;
>>> UINTN BufferSize;
>>> - UINTN OldSize;
>>> UINT8 *SingleCert;
>>> UINTN CertSize;
>>>
>>> @@ -612,10 +611,11 @@ Pkcs7GetCertificatesList (
>>>
>>> if (CtxChain != NULL) {
>>> BufferSize = sizeof (UINT8);
>>> - OldSize = BufferSize;
>>> CertBuf = NULL;
>>>
>>> for (Index = 0; ; Index++) {
>>> + UINTN OldSize;
>>> +
>>> Status = X509PopCertificate (CtxChain, &SingleCert, &CertSize);
>>> if (!Status) {
>>> break;
>>> @@ -656,10 +656,11 @@ Pkcs7GetCertificatesList (
>>>
>>> if (CtxUntrusted != NULL) {
>>> BufferSize = sizeof (UINT8);
>>> - OldSize = BufferSize;
>>> CertBuf = NULL;
>>>
>>> for (Index = 0; ; Index++) {
>>> + UINTN OldSize;
>>> +
>>> Status = X509PopCertificate (CtxUntrusted, &SingleCert, &CertSize);
>>> if (!Status) {
>>> break;
>>
>> However, many edk2 maintainers don't like tight scoping like this.)
>
> I had considered this and guessed it was probably against the coding
> style or it would have been done this way already. IMO it's better in every way.
>
> --
> Peter
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-24 8:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-20 15:10 [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations Peter Jones
2017-10-20 17:12 ` Laszlo Ersek
2017-10-20 18:21 ` Peter Jones
2017-10-23 3:02 ` Long, Qin
2017-10-24 7:51 ` Laszlo Ersek
2017-10-24 8:15 ` Long, Qin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox