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

[Svetlana Nikandrova]

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/ 
<http://cr.openjdk.java.net/%7Esnikandrova/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/ 
>> <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/20160706/6ed53bde/attachment.html>