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

[Svetlana Nikandrova]

Artem,

please see updated review:
http://cr.openjdk.java.net/~snikandrova/8022580/webrev.03/ 
<http://cr.openjdk.java.net/%7Esnikandrova/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/ 
>> <http://cr.openjdk.java.net/%7Esnikandrova/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/ 
>>>> <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/20160707/768d759e/attachment-0001.html>