[Pki-devel] [PATCH] 18-2 , 19-2 Fixes for review comments given for -- [PATCH] 18, 19 - Fixes for Guarded_By_Violation[18] and Unfinished NULL_RETURNS and RESOURCE_LEAKS[19] cases in Coverity for DogTag10

[Abhishek Koneru]

Please review the patches 18-2 and 19-2 with fixes for review comments
on Patches 18 and 19.

Regards,
Abhishek Koneru

On Fri, 2012-06-29 at 20:39 -0500, Endi Sukma Dewata wrote:
> On 6/28/2012 11:38 AM, Abhishek Koneru wrote:
> > Please review the patches 18 and 19 with fixes for
> > Guarded_By_Violation[18] and Remaining Null_Returns and ResourceLeaks
> > cases in Coverity for DogTag10.
> 
> As discussed over IRC, the GUARDED_BY_VIOLATION issue happens if a field 
> is synchronized in one location but not in other location. This can be 
> fixed by adding synchronized block, synchronizing the whole method, or 
> removing the original synchronization (if it's not actually needed). 
> However, if we make the synchronization too broad, it may include other 
> fields that weren't originally synchronized, which might trigger new 
> GUARDED_BY_VIOLATION issues.
> 
> For now I think we should focus on synchronizing the offending fields 
> only as reported by Coverity because if we start synchronizing other 
> fields it may have some unintended consequences. If there are fields 
> that should have been synchronized but they aren't, they should 
> investigated & fixed in separate tickets.
> 
> For patch #18 the following methods should be fixed to lock 'this' 
> object and synchronize the following fields only:
> 
> * ARequestNotifier.recoverPublishingQueue(): mSearchForRequests
> * CrossCertPairSubsystem.init(): mPublisherProcessor
> * CertificateRepository.setCertStatusUpdateInterval(): requestRepository
> * DBVirtualList.setSortKey(): mRegistry
> * KeyRepository.setKeyStatusUpdateInterval(): requestRepository
> * MD5.init(): buffer
> 
> For patch #19:
> 
> 1. In CertificateAuthority.java:1899,1943 it throws an exception with 
> log message instead of user message. I don't see a suitable user message 
> for this so it might be better to just re-throw the original exception 
> and let the caller handle it.
> 
> 2. In FileBasedPublisher.java:384 the fos.close() shouldn't be moved 
> because subsequent code depends on it. I think the "fos" instances in 
> this method should be handled by separate try-finally blocks.
> 
> 3. In KeyRepository.java:518 the patch checks for null recList but this 
> might not sufficient because in line 534 the recList is read again. It 
> might be better to throw EBaseException if it's null.
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-0018-2-Fixes-for-Comments-for-Patch-18.patch
Type: text/x-patch
Size: 7330 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120702/cc03b2d7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-akoneru-0019-2-Fixes-for-Patch-19-s-review-comments.patch
Type: text/x-patch
Size: 5545 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20120702/cc03b2d7/attachment-0001.bin>