[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-security-dev
Subject: Re: Code Review Reqeust, JDK-8169362, Interop automated testing with Chrome
From: Bernd Eckenfels <ecki () zusammenkunft ! net>
Date: 2016-11-10 7:19:25
Message-ID: 57A008FAA9E6E1E3.1E474A88-0393-4D69-AAEC-3A84DA326651 () mail ! outlook ! com
[Download RAW message or body]
Hello, just a Nit, but should this be clientNet/AppOutbound instead of Inbound (or \
maybe use Response instead?) in ClientHelloInterOp#run Is it Ok that there is no \
asserts? What cipher would be picked for example? ClientHelloInterOp
In 271+304 I would use try-with-resource or not close the in memory stream at all. \
Also while it is possible to use getBytes() I used to use StandardCharset.ASCII to \
make it explicit. Typo in 388 comment (delegated)
Gruss
Bernd
--
http://bernd.eckenfels.net
On Thu, Nov 10, 2016 at 4:13 AM +0100, "Xuelei Fan" <xuelei.fan@oracle.com> wrote:
Thanks for review. I will go ahead and push the changeset. The latest
update is:
http://cr.openjdk.java.net/~xuelei/8169362/webrev.01/
Xuelei
On 11/10/2016 9:41 AM, Bradford Wetmore wrote:
> Hi Xuelei,
>
> Nothing major, mostly nits and some Netbeans suggestions.
>
> On 11/9/2016 5:17 AM, Xuelei Fan wrote:
> > Hi,
> >
> > Please review this test enhancement:
> >
> > http://cr.openjdk.java.net/~xuelei/8169362/webrev.00/
> >
> > This update adds a interop new test case with Chrome ClientHello message.
>
> ClientHelloInterOp.java
> =======================
> 34: import not needed.
>
> 43/92/138/178: could be final.
>
> 265: is value of null is never used.
>
> 290: Generally prefer () groupings if you can.
>
> 316: Nit, you could combine:
>
> Certificate[] chain = new Certificate [] { keyCert };
>
> 200/211/285: jcheck problems.
>
> You could make a lot of these methods/fields private unless you plan to
> use them later.
>
>
> ClientHelloChromeInterOp.java
> =============================
> 37: import not needed.
>
> 46: could be final.
>
> I didn't check the format of the ClientHello, assuming it does what you
> need. Do you need me to do a secondary check?
>
> Ok otherwise,
>
> Brad
>
>
[Attachment #3 (text/html)]
<html><head></head><body><div>Hello, just a Nit, but should this be \
clientNet/AppOutbound instead of Inbound (or maybe use Response instead?) in \
ClientHelloInterOp#run</div><div><br></div><div>Is it Ok that there is no asserts? \
What cipher would be picked for \
example?</div><div><br></div><div>ClientHelloInterOp </div><div><br></div><div>In \
271+304 I would use try-with-resource or not close the in memory stream at all. Also \
while it is possible to use getBytes() I used to use StandardCharset.ASCII to make it \
explicit.</div><div><br></div><div>Typo in 388 comment (delegated)<br><br><div \
class="acompli_signature">Gruss<br>Bernd<br>-- <br><a dir="ltr" \
href="http://bernd.eckenfels.net" x-apple-data-detectors="true" \
x-apple-data-detectors-type="link" \
x-apple-data-detectors-result="0">http://bernd.eckenfels.net</a></div><br></div><br><br><br>
<div class="gmail_quote">On Thu, Nov 10, 2016 at 4:13 AM +0100, "Xuelei Fan" <span \
dir="ltr"><<a href="mailto:xuelei.fan@oracle.com" \
target="_blank">xuelei.fan@oracle.com</a>></span> wrote:<br> <br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
<div dir="3D"ltr"">
<pre>Thanks for review. I will go ahead and push the changeset. The latest
update is:
http://cr.openjdk.java.net/~xuelei/8169362/webrev.01/
Xuelei
On 11/10/2016 9:41 AM, Bradford Wetmore wrote:
> Hi Xuelei,
>
> Nothing major, mostly nits and some Netbeans suggestions.
>
> On 11/9/2016 5:17 AM, Xuelei Fan wrote:
>> Hi,
>>
>> Please review this test enhancement:
>>
>> http://cr.openjdk.java.net/~xuelei/8169362/webrev.00/
>>
>> This update adds a interop new test case with Chrome ClientHello message.
>
> ClientHelloInterOp.java
> =======================
> 34: import not needed.
>
> 43/92/138/178: could be final.
>
> 265: is value of null is never used.
>
> 290: Generally prefer () groupings if you can.
>
> 316: Nit, you could combine:
>
> Certificate[] chain = new Certificate [] { keyCert };
>
> 200/211/285: jcheck problems.
>
> You could make a lot of these methods/fields private unless you plan to
> use them later.
>
>
> ClientHelloChromeInterOp.java
> =============================
> 37: import not needed.
>
> 46: could be final.
>
> I didn't check the format of the ClientHello, assuming it does what you
> need. Do you need me to do a secondary check?
>
> Ok otherwise,
>
> Brad
>
>
</pre>
</div>
</blockquote>
</div>
</body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic