<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>