* [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