5:25 p.m.
Please find attached the patch with fixes for some 40-50 defects of type
NULL_RETURNS in Coverity for Dog Tag 10.
Please review the patch
The build passed Smoke Test.
Thank you.
Regards,
Abhishek Koneru
12:20 p.m.
On 5/31/2012 5:25 PM, Abhishek Koneru wrote:
Please find attached the patch with fixes for some 40-50 defects of
type
NULL_RETURNS in Coverity for Dog Tag 10.
Please review the patch
The build passed Smoke Test.
Generally in Java a method should throw an exception if it encounters an
error so the caller can know what the problem is and can handle it
properly. A method should return null only if it couldn't find the
object it's looking for. Some of the current code are doing this in
C-style, returning null on error, so the caller will have to explicitly
handle it. Fixing this behavior will require a major change, so for now
we'd have to stick with the current behavior, but we should have a
discussion about this.
1. In CAService.java:1503 the serialNoArray could be null if the serial
number is not found or invalid. The exception message should say that
too. To be consistent it should throw ECAException with user message
CMS_CA_MISSING_SERIAL_NUMBER.
Ideally the getExtDataInBigIntegerArray() should throw an exception on
invalid data.
2. In CRLIssuingPoint.java:1929,1949 the original code would throw an
exception if the return value is null. The new code logs the error but
continues processing the next request. I think they should throw an
exception like in #1. Or, if the error is supposed to be ignored, please
add a comment there.
Ideally the getExtDataInRevokedCertArray() should throw an exception on
invalid data.
3. In CRLIssuingPoint.java:1949-1957 the code needs to be formatted
properly. You can use Eclipse to auto format that section.
4. In StatsEvent.java:37 the mSubEvents should have been a Map instead
of a Vector so we can look up by name easily. Please open a ticket for
this, or feel free to include it in the next patch.
5. In CrossCertPairSubsystem.java:191,405 the code will throw an
exception if getConn() returns null. Instead of checking for the return
value in all invocations, it would be better to do it inside getConn()
itself.
Ideally the LdapBoundConnFactory.getConn() should throw an exception if
it cannot create a connection.
6. In HttpPKIMessage.java:77 instead of checking for null return value
it might be better to change the RequestTransfer
.getTransferAttributes() to always return an array (which could be empty
but not null) like this:
return v.toArray(new String[v.size()]);
This way the change in RequestTransfer.java:110 is no longer needed.
7. There's a typo in DBRegistry.java:460. To be consistent it should
throw EDBException. The message should be added into
base/common/src/UserMessages.properties, something like this:
CMS_DBS_MISSING_OBJECT_CLASS=Missing object class
While you're at it.. :) the code in 472-481 could be simplified into:
String[] s = attr.getStringValueArray();
Feel free to include it in your next patch or just file a ticket.
8. The LdapPredicateParser.nextToken() should never return null, so
instead of checking the return value you can throw the exception in line
330.
Same thing with PolicyPredicateParser.java:331.
9. In PublisherProcessor.java:495,539 instead of checking for null
getType(), the comparison can be changed like this:
publishingType.equals(rule.getType())
Also the loop itself can be simplified as follows:
Enumeration<ILdapRule> e = mRuleInsts.elements();
while (e.hasMoreElements()) {
LdapRule rule = (LdapRule)e.nextElement();
...
}
This way we don't need to worry about null name (which shouldn't happen
anyway) so getRules() won't need to return null.
10. In LogSubsystem.java:206 the code was changed from catching
EBaseException to catching all Exception. Any reason for that? This
might be too broad, we should let real errors (e.g. RuntimeException) be
handled by the caller. You can leave the e.printStackTrace() there if
you want.
11. In CertificateInfo.java:197 if sigAlgId == null and algm == null it
will not create new AlgorithmId so the sigAlgId will remain null, which
is not what we want. The original code would have thrown an Exception. I
think you should check the KeyCertUtil.getSigningAlgorithm() in line
191, if it returns null you should throw NoSuchAlgorithmException.
12. Similar to #5, in UGSubsystem.java instead of checking each
getConn() invocation you can check it inside getConn() itself and throw
the exception there.
13. In EnrollmentService.java:738,791 you should use user message
CMS_BASE_CERT_NOT_FOUND. The message itself in UserMessages.properties
should be changed to:
Certificate not found or invalid
Similarly, ideally the getExtDataInCertInfoArray() should throw an
exception on invalid data.
14. In HttpMessage.java:120 the readLine() will return null if it
reaches EOF. There's already a code there to detect this, but I'm not
sure why it was commented out. I think we should uncomment it.
15. In PrettyPrintFormat.toHexString() the original code would throw an
exception if the input byte array is empty. In the new code it will
return a string that contain indentations only (line 122). Also there's
still a possibility of an exception in line 115 if in == null and
lineLen == 0.
It might be better to just check the array at the beginning of the
method and throw an exception. Or don't change anything, it will throw
an exception too. Was this problem reported by Coverity as NULL_RETURNS?
I don't see any method returning null value here.
16. In KeyUsageExtension.java:213 the original constructor will fail if
it couldn't get the bitString from the value. Now it will construct the
KeyUsageExtension with null bitString. It might be better to check the
return value of getUnalignedBitString() and throw IOException("Invalid
bit string") or something like that.
--
Endi S. Dewata
1:43 p.m.
New subject: [PATCH] for Re: [PATCH] Fixes for Some of the Coverity Issues under NULL_RETURNS category for Dog Tag 10 for Review
Please find attached the fixes addressing all the review comments.
The build passed the smoke test.
Regards,
Abhishek Koneru
On Fri, 2012-06-01 at 12:20 -0500, Endi Sukma Dewata wrote:
On 5/31/2012 5:25 PM, Abhishek Koneru wrote:
> Please find attached the patch with fixes for some 40-50 defects of type
> NULL_RETURNS in Coverity for Dog Tag 10.
> Please review the patch
> The build passed Smoke Test.
Generally in Java a method should throw an exception if it encounters an
error so the caller can know what the problem is and can handle it
properly. A method should return null only if it couldn't find the
object it's looking for. Some of the current code are doing this in
C-style, returning null on error, so the caller will have to explicitly
handle it. Fixing this behavior will require a major change, so for now
we'd have to stick with the current behavior, but we should have a
discussion about this.
1. In CAService.java:1503 the serialNoArray could be null if the serial
number is not found or invalid. The exception message should say that
too. To be consistent it should throw ECAException with user message
CMS_CA_MISSING_SERIAL_NUMBER.
Ideally the getExtDataInBigIntegerArray() should throw an exception on
invalid data.
2. In CRLIssuingPoint.java:1929,1949 the original code would throw an
exception if the return value is null. The new code logs the error but
continues processing the next request. I think they should throw an
exception like in #1. Or, if the error is supposed to be ignored, please
add a comment there.
Ideally the getExtDataInRevokedCertArray() should throw an exception on
invalid data.
3. In CRLIssuingPoint.java:1949-1957 the code needs to be formatted
properly. You can use Eclipse to auto format that section.
4. In StatsEvent.java:37 the mSubEvents should have been a Map instead
of a Vector so we can look up by name easily. Please open a ticket for
this, or feel free to include it in the next patch.
5. In CrossCertPairSubsystem.java:191,405 the code will throw an
exception if getConn() returns null. Instead of checking for the return
value in all invocations, it would be better to do it inside getConn()
itself.
Ideally the LdapBoundConnFactory.getConn() should throw an exception if
it cannot create a connection.
6. In HttpPKIMessage.java:77 instead of checking for null return value
it might be better to change the RequestTransfer
.getTransferAttributes() to always return an array (which could be empty
but not null) like this:
return v.toArray(new String[v.size()]);
This way the change in RequestTransfer.java:110 is no longer needed.
7. There's a typo in DBRegistry.java:460. To be consistent it should
throw EDBException. The message should be added into
base/common/src/UserMessages.properties, something like this:
CMS_DBS_MISSING_OBJECT_CLASS=Missing object class
While you're at it.. :) the code in 472-481 could be simplified into:
String[] s = attr.getStringValueArray();
Feel free to include it in your next patch or just file a ticket.
8. The LdapPredicateParser.nextToken() should never return null, so
instead of checking the return value you can throw the exception in line
330.
Same thing with PolicyPredicateParser.java:331.
9. In PublisherProcessor.java:495,539 instead of checking for null
getType(), the comparison can be changed like this:
publishingType.equals(rule.getType())
Also the loop itself can be simplified as follows:
Enumeration<ILdapRule> e = mRuleInsts.elements();
while (e.hasMoreElements()) {
LdapRule rule = (LdapRule)e.nextElement();
...
}
This way we don't need to worry about null name (which shouldn't happen
anyway) so getRules() won't need to return null.
10. In LogSubsystem.java:206 the code was changed from catching
EBaseException to catching all Exception. Any reason for that? This
might be too broad, we should let real errors (e.g. RuntimeException) be
handled by the caller. You can leave the e.printStackTrace() there if
you want.
11. In CertificateInfo.java:197 if sigAlgId == null and algm == null it
will not create new AlgorithmId so the sigAlgId will remain null, which
is not what we want. The original code would have thrown an Exception. I
think you should check the KeyCertUtil.getSigningAlgorithm() in line
191, if it returns null you should throw NoSuchAlgorithmException.
12. Similar to #5, in UGSubsystem.java instead of checking each
getConn() invocation you can check it inside getConn() itself and throw
the exception there.
13. In EnrollmentService.java:738,791 you should use user message
CMS_BASE_CERT_NOT_FOUND. The message itself in UserMessages.properties
should be changed to:
Certificate not found or invalid
Similarly, ideally the getExtDataInCertInfoArray() should throw an
exception on invalid data.
14. In HttpMessage.java:120 the readLine() will return null if it
reaches EOF. There's already a code there to detect this, but I'm not
sure why it was commented out. I think we should uncomment it.
15. In PrettyPrintFormat.toHexString() the original code would throw an
exception if the input byte array is empty. In the new code it will
return a string that contain indentations only (line 122). Also there's
still a possibility of an exception in line 115 if in == null and
lineLen == 0.
It might be better to just check the array at the beginning of the
method and throw an exception. Or don't change anything, it will throw
an exception too. Was this problem reported by Coverity as NULL_RETURNS?
I don't see any method returning null value here.
16. In KeyUsageExtension.java:213 the original constructor will fail if
it couldn't get the bitString from the value. Now it will construct the
KeyUsageExtension with null bitString. It might be better to check the
return value of getUnalignedBitString() and throw IOException("Invalid
bit string") or something like that.
3:10 p.m.
New subject: [PATCH]10-2 Fixes for Some of the Coverity Issues under NULL_RETURNS category for Dog Tag 10 for review and commit
Please find attached the patch with fixes for issues in the previous
patch[#10] for review.
--Abhishek Koneru
On Mon, 2012-06-04 at 14:43 -0400, Abhishek Koneru wrote:
Please find attached the fixes addressing all the review comments.
The build passed the smoke test.
Regards,
Abhishek Koneru
On Fri, 2012-06-01 at 12:20 -0500, Endi Sukma Dewata wrote:
> On 5/31/2012 5:25 PM, Abhishek Koneru wrote:
> > Please find attached the patch with fixes for some 40-50 defects of type
> > NULL_RETURNS in Coverity for Dog Tag 10.
> > Please review the patch
> > The build passed Smoke Test.
>
> Generally in Java a method should throw an exception if it encounters an
> error so the caller can know what the problem is and can handle it
> properly. A method should return null only if it couldn't find the
> object it's looking for. Some of the current code are doing this in
> C-style, returning null on error, so the caller will have to explicitly
> handle it. Fixing this behavior will require a major change, so for now
> we'd have to stick with the current behavior, but we should have a
> discussion about this.
>
> 1. In CAService.java:1503 the serialNoArray could be null if the serial
> number is not found or invalid. The exception message should say that
> too. To be consistent it should throw ECAException with user message
> CMS_CA_MISSING_SERIAL_NUMBER.
>
> Ideally the getExtDataInBigIntegerArray() should throw an exception on
> invalid data.
>
> 2. In CRLIssuingPoint.java:1929,1949 the original code would throw an
> exception if the return value is null. The new code logs the error but
> continues processing the next request. I think they should throw an
> exception like in #1. Or, if the error is supposed to be ignored, please
> add a comment there.
>
> Ideally the getExtDataInRevokedCertArray() should throw an exception on
> invalid data.
>
> 3. In CRLIssuingPoint.java:1949-1957 the code needs to be formatted
> properly. You can use Eclipse to auto format that section.
>
> 4. In StatsEvent.java:37 the mSubEvents should have been a Map instead
> of a Vector so we can look up by name easily. Please open a ticket for
> this, or feel free to include it in the next patch.
>
> 5. In CrossCertPairSubsystem.java:191,405 the code will throw an
> exception if getConn() returns null. Instead of checking for the return
> value in all invocations, it would be better to do it inside getConn()
> itself.
>
> Ideally the LdapBoundConnFactory.getConn() should throw an exception if
> it cannot create a connection.
>
> 6. In HttpPKIMessage.java:77 instead of checking for null return value
> it might be better to change the RequestTransfer
> .getTransferAttributes() to always return an array (which could be empty
> but not null) like this:
>
> return v.toArray(new String[v.size()]);
>
> This way the change in RequestTransfer.java:110 is no longer needed.
>
> 7. There's a typo in DBRegistry.java:460. To be consistent it should
> throw EDBException. The message should be added into
> base/common/src/UserMessages.properties, something like this:
>
> CMS_DBS_MISSING_OBJECT_CLASS=Missing object class
>
> While you're at it.. :) the code in 472-481 could be simplified into:
>
> String[] s = attr.getStringValueArray();
>
> Feel free to include it in your next patch or just file a ticket.
>
> 8. The LdapPredicateParser.nextToken() should never return null, so
> instead of checking the return value you can throw the exception in line
> 330.
>
> Same thing with PolicyPredicateParser.java:331.
>
> 9. In PublisherProcessor.java:495,539 instead of checking for null
> getType(), the comparison can be changed like this:
>
> publishingType.equals(rule.getType())
>
> Also the loop itself can be simplified as follows:
>
> Enumeration<ILdapRule> e = mRuleInsts.elements();
> while (e.hasMoreElements()) {
> LdapRule rule = (LdapRule)e.nextElement();
> ...
> }
>
> This way we don't need to worry about null name (which shouldn't happen
> anyway) so getRules() won't need to return null.
>
> 10. In LogSubsystem.java:206 the code was changed from catching
> EBaseException to catching all Exception. Any reason for that? This
> might be too broad, we should let real errors (e.g. RuntimeException) be
> handled by the caller. You can leave the e.printStackTrace() there if
> you want.
>
> 11. In CertificateInfo.java:197 if sigAlgId == null and algm == null it
> will not create new AlgorithmId so the sigAlgId will remain null, which
> is not what we want. The original code would have thrown an Exception. I
> think you should check the KeyCertUtil.getSigningAlgorithm() in line
> 191, if it returns null you should throw NoSuchAlgorithmException.
>
> 12. Similar to #5, in UGSubsystem.java instead of checking each
> getConn() invocation you can check it inside getConn() itself and throw
> the exception there.
>
> 13. In EnrollmentService.java:738,791 you should use user message
> CMS_BASE_CERT_NOT_FOUND. The message itself in UserMessages.properties
> should be changed to:
>
> Certificate not found or invalid
>
> Similarly, ideally the getExtDataInCertInfoArray() should throw an
> exception on invalid data.
>
> 14. In HttpMessage.java:120 the readLine() will return null if it
> reaches EOF. There's already a code there to detect this, but I'm not
> sure why it was commented out. I think we should uncomment it.
>
> 15. In PrettyPrintFormat.toHexString() the original code would throw an
> exception if the input byte array is empty. In the new code it will
> return a string that contain indentations only (line 122). Also there's
> still a possibility of an exception in line 115 if in == null and
> lineLen == 0.
>
> It might be better to just check the array at the beginning of the
> method and throw an exception. Or don't change anything, it will throw
> an exception too. Was this problem reported by Coverity as NULL_RETURNS?
> I don't see any method returning null value here.
>
> 16. In KeyUsageExtension.java:213 the original constructor will fail if
> it couldn't get the bitString from the value. Now it will construct the
> KeyUsageExtension with null bitString. It might be better to check the
> return value of getUnalignedBitString() and throw IOException("Invalid
> bit string") or something like that.
>
11:23 a.m.
New subject: [PATCH]10-3 Fixes for Some of the Coverity Issues under NULL_RETURNS category for Dog Tag 10 for review and commit
Please find attached [PATCH] [10-3] for review.
Regards,
Abhishsek Koneru
On Mon, 2012-06-04 at 16:10 -0400, Abhishek Koneru wrote:
Please find attached the patch with fixes for issues in the previous
patch[#10] for review.
--Abhishek Koneru
On Mon, 2012-06-04 at 14:43 -0400, Abhishek Koneru wrote:
> Please find attached the fixes addressing all the review comments.
> The build passed the smoke test.
>
> Regards,
> Abhishek Koneru
>
> On Fri, 2012-06-01 at 12:20 -0500, Endi Sukma Dewata wrote:
> > On 5/31/2012 5:25 PM, Abhishek Koneru wrote:
> > > Please find attached the patch with fixes for some 40-50 defects of type
> > > NULL_RETURNS in Coverity for Dog Tag 10.
> > > Please review the patch
> > > The build passed Smoke Test.
> >
> > Generally in Java a method should throw an exception if it encounters an
> > error so the caller can know what the problem is and can handle it
> > properly. A method should return null only if it couldn't find the
> > object it's looking for. Some of the current code are doing this in
> > C-style, returning null on error, so the caller will have to explicitly
> > handle it. Fixing this behavior will require a major change, so for now
> > we'd have to stick with the current behavior, but we should have a
> > discussion about this.
> >
> > 1. In CAService.java:1503 the serialNoArray could be null if the serial
> > number is not found or invalid. The exception message should say that
> > too. To be consistent it should throw ECAException with user message
> > CMS_CA_MISSING_SERIAL_NUMBER.
> >
> > Ideally the getExtDataInBigIntegerArray() should throw an exception on
> > invalid data.
> >
> > 2. In CRLIssuingPoint.java:1929,1949 the original code would throw an
> > exception if the return value is null. The new code logs the error but
> > continues processing the next request. I think they should throw an
> > exception like in #1. Or, if the error is supposed to be ignored, please
> > add a comment there.
> >
> > Ideally the getExtDataInRevokedCertArray() should throw an exception on
> > invalid data.
> >
> > 3. In CRLIssuingPoint.java:1949-1957 the code needs to be formatted
> > properly. You can use Eclipse to auto format that section.
> >
> > 4. In StatsEvent.java:37 the mSubEvents should have been a Map instead
> > of a Vector so we can look up by name easily. Please open a ticket for
> > this, or feel free to include it in the next patch.
> >
> > 5. In CrossCertPairSubsystem.java:191,405 the code will throw an
> > exception if getConn() returns null. Instead of checking for the return
> > value in all invocations, it would be better to do it inside getConn()
> > itself.
> >
> > Ideally the LdapBoundConnFactory.getConn() should throw an exception if
> > it cannot create a connection.
> >
> > 6. In HttpPKIMessage.java:77 instead of checking for null return value
> > it might be better to change the RequestTransfer
> > .getTransferAttributes() to always return an array (which could be empty
> > but not null) like this:
> >
> > return v.toArray(new String[v.size()]);
> >
> > This way the change in RequestTransfer.java:110 is no longer needed.
> >
> > 7. There's a typo in DBRegistry.java:460. To be consistent it should
> > throw EDBException. The message should be added into
> > base/common/src/UserMessages.properties, something like this:
> >
> > CMS_DBS_MISSING_OBJECT_CLASS=Missing object class
> >
> > While you're at it.. :) the code in 472-481 could be simplified into:
> >
> > String[] s = attr.getStringValueArray();
> >
> > Feel free to include it in your next patch or just file a ticket.
> >
> > 8. The LdapPredicateParser.nextToken() should never return null, so
> > instead of checking the return value you can throw the exception in line
> > 330.
> >
> > Same thing with PolicyPredicateParser.java:331.
> >
> > 9. In PublisherProcessor.java:495,539 instead of checking for null
> > getType(), the comparison can be changed like this:
> >
> > publishingType.equals(rule.getType())
> >
> > Also the loop itself can be simplified as follows:
> >
> > Enumeration<ILdapRule> e = mRuleInsts.elements();
> > while (e.hasMoreElements()) {
> > LdapRule rule = (LdapRule)e.nextElement();
> > ...
> > }
> >
> > This way we don't need to worry about null name (which shouldn't happen
> > anyway) so getRules() won't need to return null.
> >
> > 10. In LogSubsystem.java:206 the code was changed from catching
> > EBaseException to catching all Exception. Any reason for that? This
> > might be too broad, we should let real errors (e.g. RuntimeException) be
> > handled by the caller. You can leave the e.printStackTrace() there if
> > you want.
> >
> > 11. In CertificateInfo.java:197 if sigAlgId == null and algm == null it
> > will not create new AlgorithmId so the sigAlgId will remain null, which
> > is not what we want. The original code would have thrown an Exception. I
> > think you should check the KeyCertUtil.getSigningAlgorithm() in line
> > 191, if it returns null you should throw NoSuchAlgorithmException.
> >
> > 12. Similar to #5, in UGSubsystem.java instead of checking each
> > getConn() invocation you can check it inside getConn() itself and throw
> > the exception there.
> >
> > 13. In EnrollmentService.java:738,791 you should use user message
> > CMS_BASE_CERT_NOT_FOUND. The message itself in UserMessages.properties
> > should be changed to:
> >
> > Certificate not found or invalid
> >
> > Similarly, ideally the getExtDataInCertInfoArray() should throw an
> > exception on invalid data.
> >
> > 14. In HttpMessage.java:120 the readLine() will return null if it
> > reaches EOF. There's already a code there to detect this, but I'm not
> > sure why it was commented out. I think we should uncomment it.
> >
> > 15. In PrettyPrintFormat.toHexString() the original code would throw an
> > exception if the input byte array is empty. In the new code it will
> > return a string that contain indentations only (line 122). Also there's
> > still a possibility of an exception in line 115 if in == null and
> > lineLen == 0.
> >
> > It might be better to just check the array at the beginning of the
> > method and throw an exception. Or don't change anything, it will throw
> > an exception too. Was this problem reported by Coverity as NULL_RETURNS?
> > I don't see any method returning null value here.
> >
> > 16. In KeyUsageExtension.java:213 the original constructor will fail if
> > it couldn't get the bitString from the value. Now it will construct the
> > KeyUsageExtension with null bitString. It might be better to check the
> > return value of getUnalignedBitString() and throw IOException("Invalid
> > bit string") or something like that.
> >
>
2:18 p.m.
New subject: [PATCH]10-4 -- Minor Fix in [PATCH] 10-3
Please find attached a patch with a minor fix in CertificateInfo class
for review.
Regards,
Abhishek Koneru
On Tue, 2012-06-05 at 12:23 -0400, Abhishek Koneru wrote:
Please find attached [PATCH] [10-3] for review.
Regards,
Abhishsek Koneru
On Mon, 2012-06-04 at 16:10 -0400, Abhishek Koneru wrote:
> Please find attached the patch with fixes for issues in the previous
> patch[#10] for review.
>
> --Abhishek Koneru
>
> On Mon, 2012-06-04 at 14:43 -0400, Abhishek Koneru wrote:
> > Please find attached the fixes addressing all the review comments.
> > The build passed the smoke test.
> >
> > Regards,
> > Abhishek Koneru
> >
> > On Fri, 2012-06-01 at 12:20 -0500, Endi Sukma Dewata wrote:
> > > On 5/31/2012 5:25 PM, Abhishek Koneru wrote:
> > > > Please find attached the patch with fixes for some 40-50 defects of
type
> > > > NULL_RETURNS in Coverity for Dog Tag 10.
> > > > Please review the patch
> > > > The build passed Smoke Test.
> > >
> > > Generally in Java a method should throw an exception if it encounters an
> > > error so the caller can know what the problem is and can handle it
> > > properly. A method should return null only if it couldn't find the
> > > object it's looking for. Some of the current code are doing this in
> > > C-style, returning null on error, so the caller will have to explicitly
> > > handle it. Fixing this behavior will require a major change, so for now
> > > we'd have to stick with the current behavior, but we should have a
> > > discussion about this.
> > >
> > > 1. In CAService.java:1503 the serialNoArray could be null if the serial
> > > number is not found or invalid. The exception message should say that
> > > too. To be consistent it should throw ECAException with user message
> > > CMS_CA_MISSING_SERIAL_NUMBER.
> > >
> > > Ideally the getExtDataInBigIntegerArray() should throw an exception on
> > > invalid data.
> > >
> > > 2. In CRLIssuingPoint.java:1929,1949 the original code would throw an
> > > exception if the return value is null. The new code logs the error but
> > > continues processing the next request. I think they should throw an
> > > exception like in #1. Or, if the error is supposed to be ignored, please
> > > add a comment there.
> > >
> > > Ideally the getExtDataInRevokedCertArray() should throw an exception on
> > > invalid data.
> > >
> > > 3. In CRLIssuingPoint.java:1949-1957 the code needs to be formatted
> > > properly. You can use Eclipse to auto format that section.
> > >
> > > 4. In StatsEvent.java:37 the mSubEvents should have been a Map instead
> > > of a Vector so we can look up by name easily. Please open a ticket for
> > > this, or feel free to include it in the next patch.
> > >
> > > 5. In CrossCertPairSubsystem.java:191,405 the code will throw an
> > > exception if getConn() returns null. Instead of checking for the return
> > > value in all invocations, it would be better to do it inside getConn()
> > > itself.
> > >
> > > Ideally the LdapBoundConnFactory.getConn() should throw an exception if
> > > it cannot create a connection.
> > >
> > > 6. In HttpPKIMessage.java:77 instead of checking for null return value
> > > it might be better to change the RequestTransfer
> > > .getTransferAttributes() to always return an array (which could be empty
> > > but not null) like this:
> > >
> > > return v.toArray(new String[v.size()]);
> > >
> > > This way the change in RequestTransfer.java:110 is no longer needed.
> > >
> > > 7. There's a typo in DBRegistry.java:460. To be consistent it should
> > > throw EDBException. The message should be added into
> > > base/common/src/UserMessages.properties, something like this:
> > >
> > > CMS_DBS_MISSING_OBJECT_CLASS=Missing object class
> > >
> > > While you're at it.. :) the code in 472-481 could be simplified into:
> > >
> > > String[] s = attr.getStringValueArray();
> > >
> > > Feel free to include it in your next patch or just file a ticket.
> > >
> > > 8. The LdapPredicateParser.nextToken() should never return null, so
> > > instead of checking the return value you can throw the exception in line
> > > 330.
> > >
> > > Same thing with PolicyPredicateParser.java:331.
> > >
> > > 9. In PublisherProcessor.java:495,539 instead of checking for null
> > > getType(), the comparison can be changed like this:
> > >
> > > publishingType.equals(rule.getType())
> > >
> > > Also the loop itself can be simplified as follows:
> > >
> > > Enumeration<ILdapRule> e = mRuleInsts.elements();
> > > while (e.hasMoreElements()) {
> > > LdapRule rule = (LdapRule)e.nextElement();
> > > ...
> > > }
> > >
> > > This way we don't need to worry about null name (which shouldn't
happen
> > > anyway) so getRules() won't need to return null.
> > >
> > > 10. In LogSubsystem.java:206 the code was changed from catching
> > > EBaseException to catching all Exception. Any reason for that? This
> > > might be too broad, we should let real errors (e.g. RuntimeException) be
> > > handled by the caller. You can leave the e.printStackTrace() there if
> > > you want.
> > >
> > > 11. In CertificateInfo.java:197 if sigAlgId == null and algm == null it
> > > will not create new AlgorithmId so the sigAlgId will remain null, which
> > > is not what we want. The original code would have thrown an Exception. I
> > > think you should check the KeyCertUtil.getSigningAlgorithm() in line
> > > 191, if it returns null you should throw NoSuchAlgorithmException.
> > >
> > > 12. Similar to #5, in UGSubsystem.java instead of checking each
> > > getConn() invocation you can check it inside getConn() itself and throw
> > > the exception there.
> > >
> > > 13. In EnrollmentService.java:738,791 you should use user message
> > > CMS_BASE_CERT_NOT_FOUND. The message itself in UserMessages.properties
> > > should be changed to:
> > >
> > > Certificate not found or invalid
> > >
> > > Similarly, ideally the getExtDataInCertInfoArray() should throw an
> > > exception on invalid data.
> > >
> > > 14. In HttpMessage.java:120 the readLine() will return null if it
> > > reaches EOF. There's already a code there to detect this, but I'm
not
> > > sure why it was commented out. I think we should uncomment it.
> > >
> > > 15. In PrettyPrintFormat.toHexString() the original code would throw an
> > > exception if the input byte array is empty. In the new code it will
> > > return a string that contain indentations only (line 122). Also
there's
> > > still a possibility of an exception in line 115 if in == null and
> > > lineLen == 0.
> > >
> > > It might be better to just check the array at the beginning of the
> > > method and throw an exception. Or don't change anything, it will throw
> > > an exception too. Was this problem reported by Coverity as NULL_RETURNS?
> > > I don't see any method returning null value here.
> > >
> > > 16. In KeyUsageExtension.java:213 the original constructor will fail if
> > > it couldn't get the bitString from the value. Now it will construct
the
> > > KeyUsageExtension with null bitString. It might be better to check the
> > > return value of getUnalignedBitString() and throw
IOException("Invalid
> > > bit string") or something like that.
> > >
> >
>
5037
days inactive
5042
days old
6 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata