RFR (M) 8191521: handle long relative path specified in -Xbootclasspath/a on windows

Langer, Christoph christoph.langer at sap.com
Fri Sep 27 09:17:53 UTC 2019


Hi Ralf,

this looks good now, thanks for adding the explanations to the test.

Maybe you can mention os::same_files in the comments in line 470 as well, but no need for a new webrev ��

Best regards
Christoph

> -----Original Message-----
> From: Schmelter, Ralf <ralf.schmelter at sap.com>
> Sent: Freitag, 27. September 2019 11:10
> To: Langer, Christoph <christoph.langer at sap.com>
> Cc: Calvin Cheung <calvin.cheung at oracle.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: RE: RFR (M) 8191521: handle long relative path specified in -
> Xbootclasspath/a on windows
> 
> Hi Christoph,
> 
> thanks for the review. I've made a new incremental webrev
> http://cr.openjdk.java.net/~rschmelter/webrevs/8191521/webrev.3.inc/
> containing the changes to the test and LongBCP.java.
> 
> Best regards,
> Ralf
> 
> -----Original Message-----
> From: Langer, Christoph <christoph.langer at sap.com>
> Sent: Mittwoch, 25. September 2019 15:52
> To: Schmelter, Ralf <ralf.schmelter at sap.com>
> Cc: Calvin Cheung <calvin.cheung at oracle.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: RE: RFR (M) 8191521: handle long relative path specified in -
> Xbootclasspath/a on windows
> 
> Hi Ralf,
> 
> I eventually found the time to look at this change now. Sorry for the delay.
> 
> Thanks a lot for working on this patch. It'll probably fix quite some issues for
> the JDK on Windows when working with unusual path combinations.
> 
> I've looked at
> http://cr.openjdk.java.net/~rschmelter/webrevs/8191521/webrev.2/, which
> seemingly adds the check for null paths at the beginning of
> wide_abs_unc_path in os_windows.cpp. This patch version has been running
> in our nightly tests now for a long time and the results look very good - no
> obvious regressions.
> 
> As for the patch: It looks good to me in general.
> 
> os_windows.cpp: no comment.
> 
> The test in test_windows.cpp maybe deserves a few more comments to
> ease the understanding for future developers. I would move the comments
> from line 89 and 90 to line 451 where the actual test function starts. Can you
> then please elaborate a bit more that the test by default runs in the
> regression test mode (e.g. TEST) which verifies the functionality of os::stat,
> os::open and os::dir_is_empty. And furthermore there is the EXAMPLES
> mode to illustrate the behavior of certain Windows APIs and the BENCH
> mode for a simple benchmark of file operations.
> 
> I'm also wondering, if you want to test os::same_files as well (which makes
> use of the new wide_abs_unc_path method).
> 
> As for the modified testcase LongBCP.java: You should remove "import
> jdk.test.lib.Platform;" in line 42 as well. It's not needed any more after your
> change.
> 
> Thanks & Best regards
> Christoph
> 
> 
> > -----Original Message-----
> > From: hotspot-runtime-dev <hotspot-runtime-dev-
> > bounces at openjdk.java.net> On Behalf Of Calvin Cheung
> > Sent: Freitag, 26. Juli 2019 20:59
> > To: Schmelter, Ralf <ralf.schmelter at sap.com>; hotspot-runtime-
> > dev at openjdk.java.net
> > Subject: Re: RFR (M) 8191521: handle long relative path specified in -
> > Xbootclasspath/a on windows
> >
> > Hi Ralf,
> >
> > On 7/26/19 5:20 AM, Schmelter, Ralf wrote:
> > > Hi Calvin,
> > >
> > > I've updated the webrev with your suggestions. The tests now disable
> the
> > > UNC path portion, if you don't have share <DRIVE>$ for <DRIVE>:
> > > So all tests should now run on your machine.
> >
> > The update looks good and the gtest passed. I saw the following in the
> > test log:
> >
> > [----------] 2 tests from os_windows
> > [ RUN      ] os_windows.reserve_memory_special_test_vm
> > [       OK ] os_windows.reserve_memory_special_test_vm (0 ms)
> > [ RUN      ] os_windows.handle_long_paths_test_vm
> > Disabled UNC path test, since T: is not mapped as share T$.
> > [       OK ] os_windows.handle_long_paths_test_vm (13835 ms)
> > [----------] 2 tests from os_windows (13835 ms total)
> >
> > thanks,
> >
> > Calvin
> >
> > >
> > > http://cr.openjdk.java.net/~rschmelter/webrevs/8191521/webrev.1/
> > >
> > > Best regards,
> > > Ralf


More information about the hotspot-runtime-dev mailing list