RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Calvin Cheung
bug: https://bugs.openjdk.java.net/browse/JDK-8192927

This change is targeted for JDK 11.
Please refer to the comment in the bug for a description of the change.
It also handles path longer than MAX_PATH.

webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/

Testing:
     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
     appcds tests on the above platforms + sparcv9.

thanks,
Calvin
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Calvin Cheung
I've updated the change slightly:
- replaced the do-while loop with a while loop;
- free(search_path) should be os::free(search_path)

updated webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/

thanks,
Calvin

On 12/19/17, 5:01 PM, Calvin Cheung wrote:

> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>
> This change is targeted for JDK 11.
> Please refer to the comment in the bug for a description of the
> change. It also handles path longer than MAX_PATH.
>
> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>
> Testing:
>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>     appcds tests on the above platforms + sparcv9.
>
> thanks,
> Calvin
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Ioi Lam
Hi Calvin,

The code changes look good. I would suggesting running the tests in our
test harness several times so you can run on different windows hosts.


For the test case, I think there are too many use of "DoesntMatter"
which may be confusing. I would suggest adding

     String classList[] = {"java/lang/Object"};

and replacingall 'TestCommon.list("DoesntMatter")' with classList.

Also,

    File subDir = new File(longDir, "DoesntMatter");
->
    File subDir = new File(longDir, "subdir");


Thanks
- Ioi



On 12/20/17 2:00 PM, Calvin Cheung wrote:

> I've updated the change slightly:
> - replaced the do-while loop with a while loop;
> - free(search_path) should be os::free(search_path)
>
> updated webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/
>
> thanks,
> Calvin
>
> On 12/19/17, 5:01 PM, Calvin Cheung wrote:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>>
>> This change is targeted for JDK 11.
>> Please refer to the comment in the bug for a description of the
>> change. It also handles path longer than MAX_PATH.
>>
>> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>>
>> Testing:
>>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>>     appcds tests on the above platforms + sparcv9.
>>
>> thanks,
>> Calvin

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Calvin Cheung
Hi Ioi,

Thanks for your review.

I've made your suggested changes in the test case and ran it on 10
different windows hosts.

updated webrev:
     http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/

thanks,
Calvin

On 1/3/18, 6:02 PM, Ioi Lam wrote:

> Hi Calvin,
>
> The code changes look good. I would suggesting running the tests in
> our test harness several times so you can run on different windows hosts.
>
>
> For the test case, I think there are too many use of "DoesntMatter"
> which may be confusing. I would suggest adding
>
>     String classList[] = {"java/lang/Object"};
>
> and replacingall 'TestCommon.list("DoesntMatter")' with classList.
>
> Also,
>
>    File subDir = new File(longDir, "DoesntMatter");
> ->
>    File subDir = new File(longDir, "subdir");
>
>
> Thanks
> - Ioi
>
>
>
> On 12/20/17 2:00 PM, Calvin Cheung wrote:
>> I've updated the change slightly:
>> - replaced the do-while loop with a while loop;
>> - free(search_path) should be os::free(search_path)
>>
>> updated webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/
>>
>> thanks,
>> Calvin
>>
>> On 12/19/17, 5:01 PM, Calvin Cheung wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>>>
>>> This change is targeted for JDK 11.
>>> Please refer to the comment in the bug for a description of the
>>> change. It also handles path longer than MAX_PATH.
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>>>
>>> Testing:
>>>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>>>     appcds tests on the above platforms + sparcv9.
>>>
>>> thanks,
>>> Calvin
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Calvin Cheung
Hi Serguei,

Thanks for your review.

Here's an updated webrev with your suggested simplifications:

http://cr.openjdk.java.net/~ccheung/8192927/webrev.03/

thanks,
Calvin

On 1/4/18, 12:48 PM, [hidden email] wrote:

> Hi Calvin,
>
> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/src/hotspot/os/windows/os_windows.cpp.frames.html
>
> I'd suggest a minor simplification.
> 4397   bool is_empty = false;
> Set the is_empty to 'true' instead.
> Then you can remove this alternative:
> 4427   if (f == INVALID_HANDLE_VALUE) {
> 4428     is_empty = true;
> It would look like this:
>     if (f != INVALID_HANDLE_VALUE) {
>       while (is_empty&&  ::FindNextFileW(f,&fd)) {
>       . . .
> Thanks,
> Serguei
>
>
> On 1/4/18 10:33, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> Thanks for your review.
>>
>> I've made your suggested changes in the test case and ran it on 10
>> different windows hosts.
>>
>> updated webrev:
>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/
>>
>> thanks,
>> Calvin
>>
>> On 1/3/18, 6:02 PM, Ioi Lam wrote:
>>> Hi Calvin,
>>>
>>> The code changes look good. I would suggesting running the tests in
>>> our test harness several times so you can run on different windows
>>> hosts.
>>>
>>>
>>> For the test case, I think there are too many use of "DoesntMatter"
>>> which may be confusing. I would suggest adding
>>>
>>>     String classList[] = {"java/lang/Object"};
>>>
>>> and replacingall 'TestCommon.list("DoesntMatter")' with classList.
>>>
>>> Also,
>>>
>>>    File subDir = new File(longDir, "DoesntMatter");
>>> ->
>>>    File subDir = new File(longDir, "subdir");
>>>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>
>>> On 12/20/17 2:00 PM, Calvin Cheung wrote:
>>>> I've updated the change slightly:
>>>> - replaced the do-while loop with a while loop;
>>>> - free(search_path) should be os::free(search_path)
>>>>
>>>> updated webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> On 12/19/17, 5:01 PM, Calvin Cheung wrote:
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>>>>>
>>>>> This change is targeted for JDK 11.
>>>>> Please refer to the comment in the bug for a description of the
>>>>> change. It also handles path longer than MAX_PATH.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>>>>>
>>>>> Testing:
>>>>>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>>>>>     appcds tests on the above platforms + sparcv9.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Ioi Lam
Looks good!

Thanks

- Ioi


On 1/4/18 2:12 PM, Calvin Cheung wrote:

> Hi Serguei,
>
> Thanks for your review.
>
> Here's an updated webrev with your suggested simplifications:
>
> http://cr.openjdk.java.net/~ccheung/8192927/webrev.03/
>
> thanks,
> Calvin
>
> On 1/4/18, 12:48 PM, [hidden email] wrote:
>> Hi Calvin,
>>
>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/src/hotspot/os/windows/os_windows.cpp.frames.html 
>>
>>
>> I'd suggest a minor simplification.
>> 4397   bool is_empty = false;
>> Set the is_empty to 'true' instead.
>> Then you can remove this alternative:
>> 4427   if (f == INVALID_HANDLE_VALUE) {
>> 4428     is_empty = true;
>> It would look like this:
>>     if (f != INVALID_HANDLE_VALUE) {
>>       while (is_empty&&  ::FindNextFileW(f,&fd)) {
>>       . . .
>> Thanks,
>> Serguei
>>
>>
>> On 1/4/18 10:33, Calvin Cheung wrote:
>>> Hi Ioi,
>>>
>>> Thanks for your review.
>>>
>>> I've made your suggested changes in the test case and ran it on 10
>>> different windows hosts.
>>>
>>> updated webrev:
>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 1/3/18, 6:02 PM, Ioi Lam wrote:
>>>> Hi Calvin,
>>>>
>>>> The code changes look good. I would suggesting running the tests in
>>>> our test harness several times so you can run on different windows
>>>> hosts.
>>>>
>>>>
>>>> For the test case, I think there are too many use of "DoesntMatter"
>>>> which may be confusing. I would suggest adding
>>>>
>>>>     String classList[] = {"java/lang/Object"};
>>>>
>>>> and replacingall 'TestCommon.list("DoesntMatter")' with classList.
>>>>
>>>> Also,
>>>>
>>>>    File subDir = new File(longDir, "DoesntMatter");
>>>> ->
>>>>    File subDir = new File(longDir, "subdir");
>>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>
>>>> On 12/20/17 2:00 PM, Calvin Cheung wrote:
>>>>> I've updated the change slightly:
>>>>> - replaced the do-while loop with a while loop;
>>>>> - free(search_path) should be os::free(search_path)
>>>>>
>>>>> updated webrev:
>>>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>
>>>>> On 12/19/17, 5:01 PM, Calvin Cheung wrote:
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>>>>>>
>>>>>> This change is targeted for JDK 11.
>>>>>> Please refer to the comment in the bug for a description of the
>>>>>> change. It also handles path longer than MAX_PATH.
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>>>>>>
>>>>>> Testing:
>>>>>>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>>>>>>     appcds tests on the above platforms + sparcv9.
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8192927: os::dir_is_empty is incorrect on Windows

Calvin Cheung
Thanks again - Ioi and Serguei.

Calvin

On 1/4/18, 4:41 PM, Ioi Lam wrote:

> Looks good!
>
> Thanks
>
> - Ioi
>
>
> On 1/4/18 2:12 PM, Calvin Cheung wrote:
>> Hi Serguei,
>>
>> Thanks for your review.
>>
>> Here's an updated webrev with your suggested simplifications:
>>
>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.03/
>>
>> thanks,
>> Calvin
>>
>> On 1/4/18, 12:48 PM, [hidden email] wrote:
>>> Hi Calvin,
>>>
>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/src/hotspot/os/windows/os_windows.cpp.frames.html 
>>>
>>>
>>> I'd suggest a minor simplification.
>>> 4397   bool is_empty = false;
>>> Set the is_empty to 'true' instead.
>>> Then you can remove this alternative:
>>> 4427   if (f == INVALID_HANDLE_VALUE) {
>>> 4428     is_empty = true;
>>> It would look like this:
>>>     if (f != INVALID_HANDLE_VALUE) {
>>>       while (is_empty&&  ::FindNextFileW(f,&fd)) {
>>>       . . .
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 1/4/18 10:33, Calvin Cheung wrote:
>>>> Hi Ioi,
>>>>
>>>> Thanks for your review.
>>>>
>>>> I've made your suggested changes in the test case and ran it on 10
>>>> different windows hosts.
>>>>
>>>> updated webrev:
>>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.02/
>>>>
>>>> thanks,
>>>> Calvin
>>>>
>>>> On 1/3/18, 6:02 PM, Ioi Lam wrote:
>>>>> Hi Calvin,
>>>>>
>>>>> The code changes look good. I would suggesting running the tests
>>>>> in our test harness several times so you can run on different
>>>>> windows hosts.
>>>>>
>>>>>
>>>>> For the test case, I think there are too many use of
>>>>> "DoesntMatter" which may be confusing. I would suggest adding
>>>>>
>>>>>     String classList[] = {"java/lang/Object"};
>>>>>
>>>>> and replacingall 'TestCommon.list("DoesntMatter")' with classList.
>>>>>
>>>>> Also,
>>>>>
>>>>>    File subDir = new File(longDir, "DoesntMatter");
>>>>> ->
>>>>>    File subDir = new File(longDir, "subdir");
>>>>>
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>>
>>>>> On 12/20/17 2:00 PM, Calvin Cheung wrote:
>>>>>> I've updated the change slightly:
>>>>>> - replaced the do-while loop with a while loop;
>>>>>> - free(search_path) should be os::free(search_path)
>>>>>>
>>>>>> updated webrev:
>>>>>> http://cr.openjdk.java.net/~ccheung/8192927/webrev.01/
>>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>>
>>>>>> On 12/19/17, 5:01 PM, Calvin Cheung wrote:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8192927
>>>>>>>
>>>>>>> This change is targeted for JDK 11.
>>>>>>> Please refer to the comment in the bug for a description of the
>>>>>>> change. It also handles path longer than MAX_PATH.
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8192927/webrev.00/
>>>>>>>
>>>>>>> Testing:
>>>>>>>     hs-tier1, hs-tier2 on linux-x64, windows-x64, macosx
>>>>>>>     appcds tests on the above platforms + sparcv9.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>
>>>
>