<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Jamil,</p>
<p>Thanks for the review, webrev updated at
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8228835/webrev.01/">http://cr.openjdk.java.net/~valeriep/8228835/webrev.01/</a></p>
<p>More replies in line below.<br>
</p>
<div class="moz-cite-prefix">On 8/8/2019 5:30 PM, Jamil Nimeh wrote:<br>
</div>
<blockquote type="cite"
cite="mid:6e3edb28-18a1-362f-273e-5a87d2c5339f@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Hi Valerie, looks pretty good. I only had a few minor
comments.<br>
</p>
<ul>
<li>pkcs11wrapper.h</li>
<ul>
<li>210-212: Do you intend to leave these trace macros in
there but commented? It seems like they could be safely
left uncommented in case you needed to use them for
debugging.</li>
<li>388: This looks like a duplicate of line 387.</li>
</ul>
</ul>
</blockquote>
<p>Yes, I have used those trace macros quite extensively, so I left
them in and commented out. More convenient this way.</p>
<p>Removed line 388.<br>
</p>
<blockquote type="cite"
cite="mid:6e3edb28-18a1-362f-273e-5a87d2c5339f@oracle.com">
<ul>
<ul>
</ul>
<li>libj2pkcs11.c</li>
<ul>
<li>271/277 (and others) - If you know you're going to zero
the memory right off the bat you could use calloc() rather
than malloc and get the benefit of the memory being zeroed
automatically. Just a suggestion, there's nothing wrong
with the code as it is.</li>
<li>395: nit - extra space between ckpdate and ";"</li>
<li>843: unnecessary double-parenthesis</li>
</ul>
</ul>
</blockquote>
<p>Fixed.</p>
<p>Mach5 run is clean. Please let me know if you have more comments.<br>
</p>
<p>Thanks,</p>
<p>Valerie<br>
</p>
<blockquote type="cite"
cite="mid:6e3edb28-18a1-362f-273e-5a87d2c5339f@oracle.com">
<ul>
<ul>
</ul>
</ul>
<p>--Jamil<br>
</p>
<div class="moz-cite-prefix">On 8/6/19 7:28 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:86ddf8f4-7fd4-4e6d-2364-043d351cb4bc@oracle.com">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<p>Anyone have spare cycles for reviewing this? <br>
</p>
<p>The current PKCS11 JNI code for handling native mechanism and
its parameters is a bit too all over the place. To make the
tracing and verification easier, I consolidated the memory
deallocation to the freeCKMechanismPtr(...) method in
p11_util.c (some was in p11_keymgmt.c). <br>
</p>
<p>Also, fixed the j<u>XXX</u>ToCK<u>XXXX</u> methods in
p11_convert.c to allocate the memory inside each method and
return NULL upon error and make sure allocated memories are
free'd upon any failure.<br>
</p>
<p>Bug: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8228835"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8228835</a></p>
<p>Webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~valeriep/8228835/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~valeriep/8228835/webrev.00/</a></p>
<p>Mach5 run is clean.</p>
Thanks,<br>
Valerie<br>
</blockquote>
</blockquote>
</body>
</html>