[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

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.