<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Max,<br>
<br>
Thanks for reviewing the webrev, below is another updated webrev
which contains fixes for all your comments, please let me know if
you have any other suggestions. <br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~amjiang/8048357/webrev.03/">http://cr.openjdk.java.net/~amjiang/8048357/webrev.03/</a><br>
<br>
Thanks,<br>
Amanda<br>
<br>
<div class="moz-cite-prefix">On 15/11/15 下午11:41, Wang Weijun wrote:<br>
</div>
<blockquote
cite="mid:1EDF0974-5372-4F24-A8E0-3A036D146F09@oracle.com"
type="cite">
<pre wrap="">Why "othervm" for all these tests?
PKCS10AttrOrder.java:
- Is sun.security.provider and sun.misc used anywhere? You have them in @modules.
- sun.security.pkcs.PKCS9Attribute contains public constants like CONTENT_TYPE_OID, SIGNING_TIME_OID, CHALLENGE_PASSWORD_OID. You can directly use them. Same as PKCS10AttributeReader.java.
- What does "order" mean in the test name? PKCS10 Attributes is a Set, therefore there is no order.
- You print out "List of attributes in constructed PKCS10 request: " but I see no content.
- checkAttributes() ensures every attribute in the encoded PKCS10 has the correct value. But you haven't checked if all entries in constructedMap are encoded. Maybe compare the size?
PKCS10AttributeReader.java:
- sun.misc in @modules?
- Line 45: .asn1 -> asn.1
- Please also compare the size of RequestStander and eReq.
PKCS8Test.java:
- sun.security.x509 in @modules?</pre>
</blockquote>
This test uses "sun.misc.HexDumpEncoder", so that's why
security.x509 is in @modules.
<meta charset="utf-8">
<blockquote
cite="mid:1EDF0974-5372-4F24-A8E0-3A036D146F09@oracle.com"
type="cite">
<pre wrap="">
- Line 206: Dump encodedKey here? Seems too late. Why not do it before the check or at least before raiseException? Same with the other 4 debug outputs after raiseException.
- EXPECTED_ALG_ID_CHRS can be just a String with \n and \t inside.
pkcs7/*.java:
Please take a look at Bernd Eckenfels's comments on Aug 21. I fully agree with him.</pre>
</blockquote>
<blockquote
cite="mid:1EDF0974-5372-4F24-A8E0-3A036D146F09@oracle.com"
type="cite">
<pre wrap="">
Thanks
Max
----
</pre>
<blockquote type="cite">
<pre wrap="">On Nov 13, 2015, at 4:57 PM, Amanda Jiang <a class="moz-txt-link-rfc2396E" href="mailto:amanda.jiang@oracle.com"><amanda.jiang@oracle.com></a> wrote:
Hi Max,
Please check the updated webrev which address your comments, please let me know if you have any other suggestions.
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~amjiang/8048357/webrev.02/">http://cr.openjdk.java.net/~amjiang/8048357/webrev.02/</a>
Thanks,
Amanda
On 15/8/21 上午12:13, Weijun Wang wrote:
</pre>
<blockquote type="cite">
<pre wrap="">PKCS10AttrOrder.java:
- Why not inline revAttributes(), prov() and constructMap()? They are only used once. Putting the content into the main method is more clear.
- You can create separate method for the while look checks. The 2 look identical.
PKCS10AttributeReader.java:
- Is it OK to indent the data in the comment to multiple levels?
- Again, initMap() not necessary.
PKCS7VerifyTest.java:
- Not sure what this is for. The 1st and 2nd argument of the @run line are the same?
</pre>
</blockquote>
<pre wrap="">those 3 arguments are actually for two different test cases, I use two @run tags in updated webrev and add some comments in test to make it clear.
</pre>
<blockquote type="cite">
<pre wrap="">
SignerOrder.java:
- Comment on what derString1 and derString1 are.
- What do you expect verifs1.length and verifs2.length to be? Will you also check it?
</pre>
</blockquote>
<pre wrap="">Yes I do check verifs1.length and verifs2.length should be same.
121 if (verifs1.length != verifs2.length) {
122 throw new RuntimeException("Length or Original vs read-in "
123 + "should be same");
124 }
</pre>
<blockquote type="cite">
<pre wrap="">
PKCS8Test.java:
- Typo: s/recieved/received/g
Thanks
Max
On 08/21/2015 07:11 AM, Amanda Jiang wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi All,
Please be free to review new tests for conformance testing of PKCS.
bug: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8048357">https://bugs.openjdk.java.net/browse/JDK-8048357</a>
webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~amjiang/8048357/webrev.01/">http://cr.openjdk.java.net/~amjiang/8048357/webrev.01/</a>
Thanks,
Amanda
</pre>
</blockquote>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>