[Pki-devel] [PATCH 11-3] -- Re: [PATCH 11-2] -- Re: [PATCH 11] Dogtag 10 - Fixes for Coverity issues relating to NULL_RETURNS Category -- for review

On 6/12/2012 4:53 PM, Abhishek Koneru wrote: > Submitting the PATCH 11-2 with fixes for review comments given. > Build passed the smoke test. >> 1. The link to JSS jar file should have been >> /usr/share/java/jss/jss4.jar. This link is created by scripts/dev_setup. The .classpath is not fixed yet. >> 12. There's a formatting issue in DBSubsystem.java:411. Also, it would >> be better to include which entry that has a problem: >> >> throw new Exception( >> "Missing attribute " + PROP_NEXT_RANGE + " in entry " + dn); The new code shows the constant name (PROP_NEXT_RANGE) instead of the actual attribute name (nextRange): throw new Exception( "Missing Attribute PROP_NEXT_RANGE in Entry" + dn); >> 20. In RecoveryService.java:200 it might be better to throw >> EKRAException with user message CMS_KRA_INVALID_KEYRECORD. This may make >> some other changes unnecessary. The new code gets the message from LogMessages instead of UserMessages: throw new EKRAException( CMS.getLogMessage("CMS_KRA_INVALID_KEYRECORD")); Some more issues: 28. In IAuthToken.java:218 the @return tag should be fixed, the method no longer returns null on error. 29. In IService.java:47 the new @throws tag is no longer needed. 30. Formatting issue in CMCAuth.java:907. 31. In ChallengeRevocationServlet1.java:135 the @throws tag doesn't match the exception. 32. In AuthTokenTest.java:144 include an exception message, for example: Unable to get key as String array. 33. In RecoveryService.java:132 the new @throws tag is no longer needed. 34. In CMSLDAP.java:212 it should throw the original exception object. 35. In HTTPClient.java:424 the inner try-catch block is actually not needed anymore. The finally-clause (lines 432-441) can be moved to the outer try-catch block at line 447. The null assignments (lines 438-440) are not necessary because these objects will be garbage collected anyway. Usually objects that require closing are handled like this: // declare variables and initialize with null Socket socket = null; OutputStream rawos = null; BufferedOutputStream os = null; PrintStream ps = null; try { // create objects socket = new Socket(hostname, port); rawos = socket.getOutputStream(); os = new BufferedOutputStream(rawos); ps = new PrintStream(os); ... } catch (Exception e) { ... throw e; } finally { // close in reverse order and ignore any errors if (ps != null) try { ps.close(); } catch (Exception e) { e.printStackTrace(); } if (rawos != null) try { rawos.close(); } catch (Exception e) { e.printStackTrace(); } if (os != null) try { os.close(); } catch (Exception e) { e.printStackTrace(); } if (socket != null) try { socket.close(); } catch (Exception e) { e.printStackTrace(); } } You probably can just close the outermost stream (the one created last, i.e. ps), it should automatically close the other streams and the socket too. 36. Formatting issue in HTTPClient.java:1196. 37. In DerInputStream.java:115 the @throws tag doesn't match the exception.