On 2/12/2020 3:10 PM, Andrew John Hughes wrote:
I originally had this as "@since 8 MR 3" but during internal review it
was requested to use @since 8 instead. This is what was presented/voted
during the MR phase.
Ah ok. I personally think the original MR 3 version would have been
better,
I personally agree with you.
but I guess we're stuck with this then.
>>> JDK-8213009 was mainly a refactor of the MSCAPI code to enhance the
code
layout, and to a lesser degree to better support Windows-native PSS
calls using CNG (MS Windows Crypto Next Generation). The SunMSCAPI
provider is a mix of mostly CAPI (Windows Crypto API) and a few CNG
calls when CAPI couldn't support everything needed.
The SunMSCAPI changes that followed would take significant effort to
extricate these reorganization changes when backporting later changes
(e.g. jdk/jdk). We could have forward ported/merged the 8u41 code into
OpenJDK 8, but unfortunately I haven't been given a lot of cycles for
OpenJDK code.
Ok. Backporting 8213009 to make future backports easier is the route I
would prefer anyway (and we've done similar in other cases). It's just
hard to tell the motivation with everything in one patch.
Agreed, thanks.
dependency of 8213010, but again, it's not obvious why that's required
here either. Some more details on why some of the less obvious bugs are
required would be helpful here.
Ok, I've added details to a comment in JDK-8230978.
Thanks for this detailed list. It's very helpful.
Sure.
If the refactoring is necessary, a
Git-style patch which shows the moves as renames (hg diff -g) would
help, so we can see what is actually being changed in these files.
I'm a bit confused, isn't the webrev already showing this:
http://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/
---begin---
...deleted...
* src/windows/classes/sun/security/mscapi/CKey.java* /(was
src/windows/classes/sun/security/mscapi/Key.java)/
///138 lines changed: 54 ins; 64 del; 20 mod; 81 unchg/
...deleted...
* src/windows/classes/sun/security/mscapi/CSignature.java /(was
src/windows/classes/sun/security/mscapi/RSASignature.java)/
///676 lines changed: 507 ins; 77 del; 92 mod; 355 unchg/
---end---
Looking at the index.html and subsequent "Frame" for this file, I can
see both the rename and the old/new code side-by-side.
I wasn't looking at the web pages, but just at the patch file
(https://cr.openjdk.java.net/~wetmore/MR3-codereview-8u252/PSS/webrev.01/jdk.patch)
and comparing with the changesets from 11u.
I'm a hardcore webrev/frames guys, so I wouldn't have thought to closely
look for this.
I believe I've discovered a "bug" in webrev when specifying specific
revisions. I use Mercurial Queues to handle my patches. With these
revisions:
r1 = current tip
r2 = ALPN patch applied
r3 = PSS patch applied
If I have applied ALPN + PSS and I use:
% webrev -r r2 -o webrev
to generate the PSS-only changes, I don't get the git headers. If I
don't specify -r, it defaults to r1 (current tip):
% webrev -o webrev
Then I do see the git changes, but unfortunately both ALPN+PSS show up
in a unified webrev.
---begin---
8230978: Add support for RSASSA-PSS Signature algorithm (Java SE 8)
...deleted...
8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
Summary: See comment in JDK-8230978 for details in Oracle JDK 8u and
OpenJDK 8u
Reviewed-by: valeriep, weijun, coffeys, pkoppula
---end---
I'd still prefer it was something like:
Summary: Contains elements of JDK-8051408 (see comments on JDK-8230978)
the reason for this being that this changeset will then show up if
someone searches the repository for 8051408, but won't trigger the
database to create a backport issue for it.
Changed.
Did you want me to add you as a reviewer?
Please.
Done.
Brad
