RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(null)

[Artem Smotrakov]

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/ 
> <http://cr.openjdk.java.net/%7Emsolovie/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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160705/b9a64ac1/attachment.html>