RFR: 8160786: No CCC for public class java.net.http.AsyncSSlDelegate

[Michael McMahon]

Simple change. An implementation class was made public by mistake

http://cr.openjdk.java.net/~michaelm/8160786/webrev.1/

Thanks
Michael

Re: RFR: 8160786: No CCC for public class java.net.http.AsyncSSlDelegate

[Chris Hegarty]

On 11/07/16 13:42, Michael McMahon wrote:

Simple change. An implementation class was made public by mistake

http://cr.openjdk.java.net/~michaelm/8160786/webrev.1/


Reviewed.

-Chris.

Re: RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(String path) handle "null" incorrectly

[Svetlana Nikandrova]

Chris,

may be you can find a minute to review:
JBS:
https://bugs.openjdk.java.net/browse/JDK-8022580
Latest webrev:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.03/ 



Thank you,
Svetlana

On 07.07.2016 21:06, Svetlana Nikandrova wrote:

Artem,

please see updated review:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.03/ 



Thanks,
Svetlana

On 07.07.2016 19:41, Artem Smotrakov wrote:

Hi Svetlana,

Thank you for addressing the comments below. The test looks fine to 
me. Just a couple of minor comments.


1. You don't really need fields in lines 77-79.

2. try-catch block in line 86 is not really necessary as well.

It would be better to update bug subject to something more meaningful.

Artem

On 07/07/2016 08:31 AM, Svetlana Nikandrova wrote:

Artem,

thank you for suggested test improvements. Here is updated webrev:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.02/ 



Chris,

seem like you have already looked into that issue before. May be you 
can find some time to review this fix?


Thank you,
Svetlana

On 06.07.2016 20:35, Artem Smotrakov wrote:

Hi Svetlana,

Thanks for addressing my comments. Could you please take a look at 
a couple of comments about TestFtpClientNameListWithNull.java below?


1. lines 64-70: should "client" be closed in "finally" block? I 
also think it might be better to re-throw IOExceptions instead of 
ignoring them (it may hide some problems). You can just add 
"throws" to main() method.


2. line 107: you close "out" in a couple of places in 
handelClient() method, it might be better to add a "finally" block, 
and shutdown everything there.


3. lines 176-182: you can close a client socket in handelClient(). 
Since it's a test for JDK 9, you can use try-with-resources like 
the following:


public void handelClient(Socket cl) {
...
try (cl) {
...
}


(please note that it doesn't work for JDK 8)

It would reduce the code and simplify the test (you would avoid 
some "try" blocks).


I am not sure that you need to close "out" if you close the socket.

4. Typo: handelClient -> handleClient

Artem

On 07/06/2016 09:11 AM, Svetlana Nikandrova wrote:

Hi Artem,

thank you for your replay. I believe I addressed all your 
comments. Please see updated diff:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.01/ 



Thank you,
Svetlana

On 05.07.2016 20:54, Artem Smotrakov wrote:

Hi Svetlana,

I am not an official reviewer, but I have a couple comments below.

TestFtpClientNameListWithNull.java:

1. Shouldn't "commandHasArgs" be volatile? Looks like it may 
cause intermittent failures. As a sanity check, you may want to 
run the test in a loop to make sure it is stable.


2. You may want to make FtpServer implement AutoCloseable 
(terminate() method just becomes close()). Then, you can use it 
in try-with-resource block which would simplify the code (you can 
avoid a couple of nested try-catch blocks).


By the way, it might be good to make sun.net.ftp.FtpClient 
implement AutoCloseable as well, but probably it should be a 
separate enhancement.


3. line 62-63, 66: should it be in "finally" block?

4. How many connection is FtpServer supposed to handle in this 
test? If only one, it might be better to remove the "while" loop 
and "done" variable to simplify the code.


5. Is it necessary to handle a client connections in a separate 
thread? Avoiding too many threads would simplify the test.


6. The test ignores a couple of IOException, it might be better 
to fail it they occur.


7. Why is it necessary to use daemons?

8. Please use braces for "if" statements, see Java Coding 
Conventions.


FtpClient.java: please update copyright year.

Artem

On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix for bug
https://bugs.openjdk.java.net/browse/JDK-8022580
Webrev:
http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/ 



Description:
There is no handling for null path parameter in the method 
sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that 
"@param path a String containing the pathname of the directory 
to list or null for the current working directory". Changeset 
adds check that if param is null NLST will be sent without 
parameter (no-parameter default is current directory).


JPRT tested.

Thank you,
Svetlana

Re: RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(String path) handle "null" incorrectly

[Daniel Fuchs]

Hi Svetlana,

Have you thought of using a CountDownLatch (or some other
synchronization mechanism like e.g. Semaphore) to wait for
the server to be ready instead of:
  55 int port = 0;
  56 while (port == 0) {
  57 Thread.sleep(500);
  58 port = server.getPort();
  59 }
Also serverSocket needs to be volatile.

Or better yet, you could bind the server socket in
the constructor of FtpServer - which would allow you
to make serverSocket final instead of volatile, and then
you would no longer need to make FtpServer extend Thread.

best regards,

-- daniel

On 07/07/16 19:06, Svetlana Nikandrova wrote:

Artem,

please see updated review:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.03/


Thanks,
Svetlana

On 07.07.2016 19:41, Artem Smotrakov wrote:

Hi Svetlana,

Thank you for addressing the comments below. The test looks fine to
me. Just a couple of minor comments.

1. You don't really need fields in lines 77-79.

2. try-catch block in line 86 is not really necessary as well.

It would be better to update bug subject to something more meaningful.

Artem

On 07/07/2016 08:31 AM, Svetlana Nikandrova wrote:

Artem,

thank you for suggested test improvements. Here is updated webrev:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.02/


Chris,

seem like you have already looked into that issue before. May be you
can find some time to review this fix?

Thank you,
Svetlana

On 06.07.2016 20:35, Artem Smotrakov wrote:

Hi Svetlana,

Thanks for addressing my comments. Could you please take a look at a
couple of comments about TestFtpClientNameListWithNull.java below?

1. lines 64-70: should "client" be closed in "finally" block? I also
think it might be better to re-throw IOExceptions instead of
ignoring them (it may hide some problems). You can just add "throws"
to main() method.

2. line 107: you close "out" in a couple of places in handelClient()
method, it might be better to add a "finally" block, and shutdown
everything there.

3. lines 176-182: you can close a client socket in handelClient().
Since it's a test for JDK 9, you can use try-with-resources like the
following:

public void handelClient(Socket cl) {
...
try (cl) {
...
}


(please note that it doesn't work for JDK 8)

It would reduce the code and simplify the test (you would avoid some
"try" blocks).

I am not sure that you need to close "out" if you close the socket.

4. Typo: handelClient -> handleClient

Artem

On 07/06/2016 09:11 AM, Svetlana Nikandrova wrote:

Hi Artem,

thank you for your replay. I believe I addressed all your comments.
Please see updated diff:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.01/


Thank you,
Svetlana

On 05.07.2016 20:54, Artem Smotrakov wrote:

Hi Svetlana,

I am not an official reviewer, but I have a couple comments below.

TestFtpClientNameListWithNull.java:

1. Shouldn't "commandHasArgs" be volatile? Looks like it may cause
intermittent failures. As a sanity check, you may want to run the
test in a loop to make sure it is stable.

2. You may want to make FtpServer implement AutoCloseable
(terminate() method just becomes close()). Then, you can use it in
try-with-resource block which would simplify the code (you can
avoid a couple of nested try-catch blocks).

By the way, it might be good to make sun.net.ftp.FtpClient
implement AutoCloseable as well, but probably it should be a
separate enhancement.

3. line 62-63, 66: should it be in "finally" block?

4. How many connection is FtpServer supposed to handle in this
test? If only one, it might be better to remove the "while" loop
and "done" variable to simplify the code.

5. Is it necessary to handle a client connections in a separate
thread? Avoiding too many threads would simplify the test.

6. The test ignores a couple of IOException, it might be better to
fail it they occur.

7. Why is it necessary to use daemons?

8. Please use braces for "if" statements, see Java Coding Conventions.

FtpClient.java: please update copyright year.

Artem

On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote:

Hello,

please review this fix for bug
https://bugs.openjdk.java.net/browse/JDK-8022580
Webrev:
http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/


Description:
There is no handling for null path parameter in the method
sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that
"@param path a String containing the pathname of the directory to
list or null for the current working directory". Changeset adds
check that if param is null NLST will be sent without parameter
(no-parameter default is current directory).

JPRT tested.

Thank you,
Svetlana

RE: RFR (S) JDK-8160174: java.net.NetworkInterface - fixes and improvements for network interface listing

[Langer, Christoph]

Hi Chris (or anyone who is holding a stake in the NetworkInterface native 
implementation),

I've spent some time on cleaning up NetworkInterface.c and came up with a 
bigger change: http://cr.openjdk.java.net/~clanger/webrevs/8160174.2/

With this I attempted to consolidate the interface listing functionality of the 
2 main categories - one is ioctl (used on Linux IPv4, Solaris and AIX) and the 
other is getifaddrs (used on MacOS). I introduced some defines to switch 
between the implementations. I also consolidated the functionality for the 
ioctl based network interface listings by using an #ifdef section to 
distinguish between the Linux/AIX versus Solaris field and constant names.

I've spent some time testing on the platforms and in principal it works. But as 
it is a matter of taste, I'd like to ask you if you support this type of change 
or have any hints or recommendations before going forward to finalize this.

For Linux I'm also suggesting to use the getifaddrs approach - I tested and 
found it working everywhere and with this we could get rid of the 
implementation to read /proc/net for IPv6.

Furthermore I'm generally setting Null for the IPv6 broadcast address - which I 
think is common sense for IPv6.

Thanks in advance and best regards
Christoph


From: Langer, Christoph
Sent: Donnerstag, 23. Juni 2016 16:37
To: 'net-dev@openjdk.java.net' 
Subject: RFR (S) JDK-8160174: java.net.NetworkInterface - fixes and 
improvements for network interface listing

Hi,

can I please get a review the following change:
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8160174.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8160174

I made further fixes and cleanups for listing Unix type network interfaces, 
especially on AIX and Linux. I ran builds and verified also on Solaris and Mac.

Thanks
Christoph

RE: RFR (S) JDK-8160174: java.net.NetworkInterface - fixes and improvements for network interface listing

[Christos Zoulas]

On Jul 11,  1:36pm, christoph.lan...@sap.com ("Langer, Christoph") wrote:
-- Subject: RE: RFR (S) JDK-8160174: java.net.NetworkInterface - fixes and im

| Hi Chris (or anyone who is holding a stake in the NetworkInterface native i=
| mplementation),
| 
| I've spent some time on cleaning up NetworkInterface.c and came up with a b=
| igger change: http://cr.openjdk.java.net/~clanger/webrevs/8160174.2/
| 
| With this I attempted to consolidate the interface listing functionality of=
|  the 2 main categories - one is ioctl (used on Linux IPv4, Solaris and AIX)=
|  and the other is getifaddrs (used on MacOS). I introduced some defines to =
| switch between the implementations. I also consolidated the functionality f=
| or the ioctl based network interface listings by using an #ifdef section to=
|  distinguish between the Linux/AIX versus Solaris field and constant names.=
| 
| 
| I've spent some time testing on the platforms and in principal it works. Bu=
| t as it is a matter of taste, I'd like to ask you if you support this type =
| of change or have any hints or recommendations before going forward to fina=
| lize this.
| 
| For Linux I'm also suggesting to use the getifaddrs approach - I tested and=
|  found it working everywhere and with this we could get rid of the implemen=
| tation to read /proc/net for IPv6.
| 
| Furthermore I'm generally setting Null for the IPv6 broadcast address - whi=
| ch I think is common sense for IPv6.
| 

Christoph,

I think that it is probably even better to create a getifaddrs(3)
compatibility layer for the OS's the still use ioctl(2), and put
all of it in a separate file.  You can probably use the code from
one of the BSD's with minor modifications.

Best,

christos

Re: RFR JDK-8161091 Incorrect Stream.FlowControl implementation allows to send DataFrame even when window size was exhausted

[Sergey Kuksenko]

I am awfully sorry, previous fix was incorrect.
Please, review the right version: 
http://cr.openjdk.java.net/~skuksenko/jep110/8161091/webrev.01/


On 07/08/2016 02:40 PM, Sergey Kuksenko wrote:

Hi,
Could you please review the following fix for JDK-8161091?

http://cr.openjdk.java.net/~skuksenko/jep110/8161091/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8161091

Existing Stream.FlowControl implementation doesn't decrease amount of 
permits if requested amount is less than permits. That means that 
client may send any amount of data if data frame size less than window 
size.


Thank you,
Sergey Kuksenko.