On Thu, 1 Apr 2021 21:27:52 GMT, Weijun Wang <[email protected]> wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java
> line 104:
>
>> 102: * key will be chosen after the first keypair is generated.
>> 103: * @param providerName name of the provider
>> 104: * @param signerPrivateKey signer's private key
>
> Add an "(optional)" or "can be null" to the last 2 `@param`s.
Done.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1163:
>
>> 1161: }
>> 1162: doGenKeyPair(alias, dname, keyAlgName, keysize, groupName,
>> sigAlgName,
>> 1163: signerAlias);
>
> Maybe we can just keep using the old argument list. `signerAlias` is a global
> variable and it's visible everywhere.
I'd prefer to have it. It is the same as other global arguments being passed to
this method.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1927:
>
>> 1925: CertAndKeyGen keypair;
>> 1926: if (signerAlias != null) {
>> 1927: signerFlag = true;
>
> Is the `signerFlag` really useful? We can just check `signerAlias != null`.
Sure, use `signerAlias` instead.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1978:
>
>> 1976: } else {
>> 1977: certImpl = new X509CertImpl(signerCert.getEncoded());
>> 1978: }
>
> The exact same 7 lines above also appears in the code change above. Is it
> possible to combine them?
Here is mainly to locate the signer’s SKID, and moved it up to the first
occurrence of these lines of code to combine them.
> src/java.base/share/classes/sun/security/tools/keytool/Resources.java line
> 310:
>
>> 308: "Generating {0} bit {1} key pair and self-signed
>> certificate ({2}) with a validity of {3} days\n\tfor: {4}"},
>> 309:
>> {"Generating.keysize.bit.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.an.entry.signerAlias.specified.by.the.signer.option.with.a.validity.of.validality.days.for",
>> 310: "Generating {0} bit {1} key pair and a certificate
>> ({2}) issued by an entry <{3}> specified by the -signer option with a
>> validity of {4} days\n\tfor: {5}"},
>
> A little too long? Can we remove the "specified by the -signer option" words
> or even the whole "issued by an entry <{3}> specified by the -signer option"?
Removed the "specified by the -signer option”, and keep the "issued by an entry
<{3}>” that I agreed it’d be good to print out the signer’s alias in response
to your previous review comment.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3281
