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/ 
<http://cr.openjdk.java.net/%7Esnikandrova/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/ 
> <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/20160711/5e81b108/attachment-0001.html>