Is it better to append the new lines to the 2nd paragraph? Thanks Max
> On Jul 18, 2018, at 9:46 AM, Valerie Peng <valerie.p...@oracle.com> wrote: > > > Ok, let's use "must" then. I have also added the part about default > parameters. > Hope that all is clear now. > > Latest webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.03/ > Latest CSR: https://bugs.openjdk.java.net/browse/JDK-8206864 > > Thanks, > Valerie > > On 7/17/2018 5:50 PM, Weijun Wang wrote: >> >>> On Jul 18, 2018, at 8:23 AM, Valerie Peng <valerie.p...@oracle.com> wrote: >>> >>> Hi Max, >>> >>> Thanks for the suggestions. Please find comments inline. >>> >>> On 7/16/2018 7:38 PM, Weijun Wang wrote: >>>> CSR at https://bugs.openjdk.java.net/browse/JDK-8206864. >>>> >>>> - At the end of the 1st paragraph, you have now >>>> >>>>> However, for signature algorithm such as "RSASSA-PSS", it requires >>>>> parameters and the one used for signing must be supplied for verification >>>>> to succeed. It may be better to return null instead of returning >>>>> provider-specific default parameters. >>>> I suggest we don't talk about the reason (params must be the same for >>>> signing and verification), we can just say something like "If there is no >>>> provider-specific default parameters, this method should return null >>>> before user sets one". >>> Alright, I initially didn't put down the reason. But since Brad asked about >>> it, I add it to the CSR in case Joe raise the same question. I will update >>> the CSR per your suggestion. >>>> - null may be returned >>>> >>>> How about "{@code null} must be returned"? >>> How about "should"? Is there a guideline on when to use "may/should/must"? >>> Anyone knows? >> Even if there were guidelines on this for Java, I think we should just stick >> to https://tools.ietf.org/html/rfc2119, except that the capitalization is >> not necessary. >> >> "Must" is precise here. >> >>> I thought must is mostly used in mandating input arguments must not be >>> null. But don't recall it being used much for return values. >> "must return" appears 39 times in java/ and "should return" 19 (simple grep, >> no line break). >> >> --Max >> >>> Thanks, >>> Valerie >>> >>>> Everything else looks fine. >>>> >>>> Thanks >>>> Max >>>> >>>>> On Jul 17, 2018, at 9:46 AM, Valerie Peng <valerie.p...@oracle.com> wrote: >>>>> >>>>> Hi Max, >>>>> >>>>> Good catch on the SignatureSpi class and SunMSCAPI provider, I will >>>>> include them. >>>>> >>>>> As for the part about "randomly generated", I am leaning toward not >>>>> having it as I don't see a value of specifying this. >>>>> >>>>> Webrev and CSR has been updated. >>>>> >>>>> New webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.02/ >>>>> >>>>> Thanks, >>>>> Valerie >>>>> >>>>> On 7/16/2018 4:29 PM, Weijun Wang wrote: >>>>>> Valerie >>>>>> >>>>>> Would you like to retain the word "randomly generated" somewhere? >>>>>> >>>>>> Also, the SignatureSpi class needs to be updated in the same way. >>>>>> >>>>>> Can you also update the implementation in the MSCAPI Signature object? >>>>>> >>>>>> Thanks >>>>>> Max >>>>>> >>>>>>> On Jul 17, 2018, at 6:16 AM, Valerie Peng <valerie.p...@oracle.com> >>>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> No issues found and it seems ok to return null if no parameters is set. >>>>>>> Thus, I have updated the webrev and CSR accordingly. >>>>>>> >>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8206171 >>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.01/ >>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8206864 >>>>>>> >>>>>>> Thanks, >>>>>>> Valerie >>>>>>> >>>>>>> On 7/13/2018 11:05 AM, Valerie Peng wrote: >>>>>>>> Hmm, I like the idea of expanding null to cover both cases. >>>>>>>> I will explore it more and see. >>>>>>>> Thanks for the feedback, >>>>>>>> Valerie >>>>>>>> >>>>>>>> On 7/13/2018 6:56 AM, Sean Mullan wrote: >>>>>>>>> On 7/12/18 10:23 PM, Weijun Wang wrote: >>>>>>>>>>> On Jul 13, 2018, at 10:01 AM, Valerie Peng >>>>>>>>>>> <valerie.p...@oracle.com> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Max, >>>>>>>>>>> >>>>>>>>>>> The javadoc is for Signature.getParameters(), so null can be >>>>>>>>>>> returned for signature algorithms which do not use parameters, e.g. >>>>>>>>>>> SHA256withDSA. As Signature class covers all signature algorithms, >>>>>>>>>>> I am not sure about mentioning specific algorithm names as it may >>>>>>>>>>> be lengthy and even misleading unless we list out all applicable >>>>>>>>>>> algorithms. >>>>>>>>>> Sure. >>>>>>>>>> >>>>>>>>>>> The part of "default and randomly generated" is inherited from >>>>>>>>>>> existing javadoc. I think "default" in the aforementioned sentence >>>>>>>>>>> means "hardcoded values". For example, something like salt length >>>>>>>>>>> will likely have a fixed default value. Since we have no control >>>>>>>>>>> over 3rd party providers, I think we may have to keep this for >>>>>>>>>>> backward compatibility reason. For RSASSA-PSS sig algorithm, it >>>>>>>>>>> errors out if the required parameter is not given. Thus, I added >>>>>>>>>>> the sentence "If there are no provider-specific default values, the >>>>>>>>>>> underlying signature implementation may also fail". >>>>>>>>>> OK, now I understand "a combination of default and randomly >>>>>>>>>> generated" means some part of the parameter is hardcoded and some is >>>>>>>>>> random. Anyway, let's keep it unchanged. >>>>>>>>>> >>>>>>>>>> Then, how about simply "If there are no provider-specific values" >>>>>>>>>> which covers both hardcoded and random values? >>>>>>>>>> >>>>>>>>>> "the underlying signature implementation may also fail" is still not >>>>>>>>>> clear to me. If I had not read the CSR I would thought an exception >>>>>>>>>> will be thrown when update/sign is called. >>>>>>>>>> >>>>>>>>>>> As for @throws, I debated about it. The main reason for not adding >>>>>>>>>>> one is consistency. Many (or should I say most) security classes do >>>>>>>>>>> not have @throws for ProviderException. If we were to add @throws >>>>>>>>>>> ProviderException here, we should do it across the board. Besides, >>>>>>>>>>> it is recommended to NOT document runtime exceptions which callers >>>>>>>>>>> are not prepared to handle. >>>>>>>>>> I assume other getParameters() had not added it is because it >>>>>>>>>> happened they can return one. >>>>>>>>>> >>>>>>>>>> While people does not catch runtime exceptions but my understanding >>>>>>>>>> is that if you mentioned "fail" in the spec maybe it's better to add >>>>>>>>>> a @throws for it. >>>>>>>>>> >>>>>>>>>> For example, @throws SecurityException for File/Files operations. >>>>>>>>> Thinking more about this, I would be more inclined to recommend that >>>>>>>>> you change the meaning of null as the return value to cover both >>>>>>>>> cases: >>>>>>>>> >>>>>>>>> @return the parameters used with this signature, or null if this >>>>>>>>> signature does not use any parameters or does not support default or >>>>>>>>> randomly generated parameter values >>>>>>>>> >>>>>>>>> I don't think it is critical to make a distinction between these 2 >>>>>>>>> cases, because if the programmer doesn't initialize it with >>>>>>>>> parameters it will get a SignatureException anyway when it tries to >>>>>>>>> call sign or verify. >>>>>>>>> >>>>>>>>> It's not perfect, but probably the best you can do working within the >>>>>>>>> constraints of that API and minimizing compatibility risk. >>>>>>>>> >>>>>>>>> --Sean >>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> Max >>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Valerie >>>>>>>>>>> >>>>>>>>>>> On 7/10/2018 7:16 PM, Weijun Wang wrote: >>>>>>>>>>>> Hi Valerie >>>>>>>>>>>> >>>>>>>>>>>> About "it *may* return", do you mean it could also return null? My >>>>>>>>>>>> understanding is no. >>>>>>>>>>>> >>>>>>>>>>>> Is it better to clarify when the implementation "may also fail"? >>>>>>>>>>>> From the CSR, it's this method. Can you add a @throws spec to this >>>>>>>>>>>> method then? >>>>>>>>>>>> >>>>>>>>>>>> Also, I am a little confused by "default and randomly generated". >>>>>>>>>>>> Does this actually mean "default (might be randomly generated)"? >>>>>>>>>>>> The "it may" sentence mentions "default and randomly generated" >>>>>>>>>>>> but the "if there" only says "default", which sounds like the the >>>>>>>>>>>> "randomly generated" case could be different. >>>>>>>>>>>> >>>>>>>>>>>> Thanks >>>>>>>>>>>> Max >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> On Jul 11, 2018, at 5:12 AM, Valerie Peng >>>>>>>>>>>>> <valerie.p...@oracle.com> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Brad, >>>>>>>>>>>>> >>>>>>>>>>>>> Would you have time to review the fix for JDK-8206171: >>>>>>>>>>>>> Signature#getParameters for RSASSA-PSS throws ProviderException >>>>>>>>>>>>> when not initialized? >>>>>>>>>>>>> No source code changes, but just updating javadoc to mention the >>>>>>>>>>>>> possible failure case. >>>>>>>>>>>>> Otherwise, JCK team expects a parameter object or null being >>>>>>>>>>>>> returned. >>>>>>>>>>>>> I filed a CSR to track the javadoc clarification. >>>>>>>>>>>>> >>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8206171 >>>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8206171/webrev.00/ >>>>>>>>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8206864 >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Valerie >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >
