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.

On 7/2/2012 10:35 AM, Abhishek Koneru wrote: > Please review the patches 18-2 and 19-2 with fixes for review comments > on Patches 18 and 19. Some issues in patch #18-2: 1. In CrossCertPairSubsystem.java:104-107 the new code looks like this: mCa = (ICertificateAuthority) CMS.getSubsystem("ca"); synchronized (this) { mPublisherProcessor = mCa.getPublisherProcessor(); } The mCa is accessed twice, one without lock and the other with lock. I suppose Coverity will complain about it. The mCa is already synchronized in shutdown(), so we should synchronize mCa everywhere else too. synchronized (this) { mCa = (ICertificateAuthority) CMS.getSubsystem("ca"); mPublisherProcessor = mCa.getPublisherProcessor(); } 2. Formatting issue in DBVirtualList.java:311-320. For patch #19-2, let's return the "fos" variable declarations into their original locations. These variables are handling different files and there is nothing shared among them, so there is no need to have a common "fos" variable. By moving the declarations back, we minimize the scope of each variable, so it's easier to read the code or if we want to break the method into smaller methods.