<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Jamil,<br>
<br>
Thanks for the review. Good catch on the formatting typo. I have
fixed it.<br>
Removed some of the casting that you mentioned also.<br>
<br>
As for only free'ing context upon non-zero return value, it's more
for allowing the java objects to be used again without
re-initialization.<br>
When an error/exception occur, we can throw away everything.
However, when operations finish successfully, we re-use the context
for subsequent operations.<br>
The context will later be freed through explicit nativeFree(long)
call when the object is being GC'ed.<br>
<br>
Valerie<br>
<br>
On 5/12/2016 11:39 AM, Jamil Nimeh wrote:
<blockquote cite="mid:5734CDEC.9090506@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
<br>
<div class="moz-cite-prefix">On 5/12/2016 10:38 AM, Anthony
Scarpino wrote:<br>
</div>
<blockquote cite="mid:5734BF7A.5070706@oracle.com" type="cite">On
05/04/2016 07:08 PM, Valerie Peng wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Can someone help reviewing the changes for SHA-3? <br>
<br>
The result has been validated against the NIST test vectors
(for <br>
BYTE-ONLY impls, i.g. input which are multiples of bytes). <br>
The feature complete date is coming up in a week or two. So,
if this can <br>
be reviewed in a week or so, that'd be great. <br>
<br>
The changes for SUN providers are quite straight-forward, e.g.
SHA-3 <br>
digest impls based on FIPS PUB 202. <br>
As for OracleUcrypto provider, Solaris SHA-3 support is
through new <br>
libucrypto digest APIs (added in Solaris 12) instead of the
libmd. <br>
When running on Solaris 12, the new libucrypto APIs will be
used. <br>
Otherwise, libmd will be used. <br>
Changes for OracleUcrypto providers: <br>
- add JNI code for the new libucrypto digest APIs <br>
- code refactoring, e.g. move the libmd-related code to
classes with MD <br>
suffix <br>
- run-time mechanism number assignment (used to be hardcoded
values) <br>
- better error reporting <br>
<br>
RFE: <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8000415">https://bugs.openjdk.java.net/browse/JDK-8000415</a>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Evaleriep/8000415/webrev.00/">http://cr.openjdk.java.net/~valeriep/8000415/webrev.00/</a>
<br>
<br>
Thanks, <br>
Valerie <br>
</blockquote>
<br>
Just a few minor things. <br>
<br>
UcryptoProvider.java:161 <br>
Did you mean to only comment it out and not delete it? <br>
<br>
nativeFunc.c: <br>
I noticed it is checking the library functions via dlsym. You
could have a conditional check on the version of the library via
ucrypto_version, then do the dlsym calls if it is supported. <br>
<br>
<br>
I think there is a performance enhancement that I would not hold
up for this changeset. Changing the byte[]'s to long[] might
give a good performance gain. A similar change was done to
GHASH.java which resulted in a 10x performance increase. Given
the internal methods appear to use long[][], and lanes are in
longs it would probably help. But it most likely require a
change to add DigestBase.implCompress(long[], int) and
DigestBase.and implDigest(long[], int). Probably a low priority
RFE. <br>
<br>
Tony <br>
<br>
</blockquote>
Hi Valerie, just a few additional nits - not a big deal either
way:<br>
<ul>
<li>nativeCryptoMD.c:</li>
<ul>
<li>47, 53, 59, 65, etc.: You don't need to cast the pointer
from malloc since you're assigning it to a void *. The
casting only needs to happen when you go to use the pointer
with a function that expects a concrete type. I tried it
with gcc and cc with full warnings just to be sure. But
it's no problem to leave it in there as-is.</li>
<li>123, 125: I think these may be unnecessary casts, too.</li>
<li>133,135,137: pretty sure free doesn't need casts. It
likes all pointers regardless of type.</li>
</ul>
<li>nativeCrypto.c:</li>
<ul>
<li>40,42: I'm surprised you're not getting warnings on these,
as "0x0x" isn't a valid format specifier so you have a
mismatch in terms of arguments vs format specifiers. Do you
mean 0x%0x? The latter works ok.</li>
<li>357: Just a question, does ucryptoDigestFinal
automatically free the context as part of the finalize?
Otherwise you're only freeing the context on a non-zero
return value from that function. I'm assuming that the
context should be freed regardless of the outcome given the
header comment.</li>
<li>362: Since context is a pointer, better to use NULL rather
than zero.</li>
</ul>
</ul>
<p>--Jamil<br>
</p>
<br>
<br>
<br>
</blockquote>
</body>
</html>