Jaroslav, > Can you elaborate why checking for the current user being able to read > the actual library file might be wrong?
It's not applicable to this particular testcase (so I'd marked it as a nit) but a generic security rule is to always check that we deal with a regular file. Try to link any block device to libjvm.so and see what happens. -Dmitry On 2013-10-07 20:39, Jaroslav Bachorik wrote: > On 7.10.2013 16:31, Dmitry Samersoff wrote: >> Jarsolav, >> >> Looks good for me, comments below is just a nits - so fill free to >> ignore it. >> >> 1. >> As FS.getPath(TEST_JDK, "jre", "lib", LIBARCH) is the only value for >> findLibjvm parameter, It's better to create an overload function >> findLibjvm(). > > Ok. It will make the code a further bit readable. > >> >> 2. >> it's better to check for File.isFile() - readable (e.g. device) is not >> always what you whant here. > > Can you elaborate why checking for the current user being able to read > the actual library file might be wrong? > >> >> 3. It's good to try >> ARCH/libjvm.so, ARCH/server/libjvm.so, ARCH/client/libjvm.so >> in order for the possible platforms with the only vm > > Ok. > > -JB- > >> >> -Dmitry >> >> >> On 2013-10-07 18:14, Jaroslav Bachorik wrote: >>> On 19.9.2013 16:33, Jaroslav Bachorik wrote: >>>> The updated webrev: >>>> http://cr.openjdk.java.net/~jbachorik/8004926/webrev.03 >>>> >>>> I've moved some of the functionality to the testlibrary. >>>> >>>> -JB - >>>> >>>> On 12.9.2013 17:31, Jaroslav Bachorik wrote: >>>>> On 09/12/2013 05:13 PM, Dmitry Samersoff wrote: >>>>>> Jaroslav, >>>>>> >>>>>> CustomLauncherTest.java: >>>>>> >>>>>> 102: this check could be moved to switch at ll. 108 >>>>>> otherwise test fails on "sunos" and "linux" because PLATFORM remains >>>>>> unset. >>>>> Good idea. Thanks. >>>>> >>>>>> 129: I would prefer don't have pattern like this one ever in shell >>>>>> script. Could you prepare a list of VM's to check and just loop over >>>>>> it? >>>>>> It makes test better readable. Also I think nowdays we can always use >>>>>> server VM. >>>>> I tried to mirror the original shell test as closely as possible. It >>>>> would be nice if we could rely on the "server" vm only. Definitely >>>>> more >>>>> readable. >>>>> >>>>> -JB- >>>>> >>>>>> -Dmitry >>>>>> >>>>>> >>>>>> On 2013-09-12 18:17, Jaroslav Bachorik wrote: >>>>>>> On 09/12/2013 10:22 AM, Jaroslav Bachorik wrote: >>>>>>>> On 09/12/2013 10:12 AM, Chris Hegarty wrote: >>>>>>>>> On 09/12/2013 04:45 AM, David Holmes wrote: >>>>>>>>>> Hi Jaroslav, >>>>>>>>>> >>>>>>>>>> You need a copyright notice in the new file. >>>>>>>>>> >>>>>>>>>> As written this test can only run on a full JDK - so please add >>>>>>>>>> it to >>>>>>>>>> the :needs_jdk group in TEST.groups. (Does jcmd really needs to >>>>>>>>>> come >>>>>>>>>> from the test-jdk? And use the VMOPTS passed to the test?) >>>>>>>>>> >>>>>>>>>> Is there a reason this test can't run on OSX? I know it would >>>>>>>>>> need >>>>>>>>>> further modification but was wondering if there is something >>>>>>>>>> inherent in >>>>>>>>>> the test that makes it inapplicable to OSX. >>>>>>>>>> >>>>>>>>>> I think the test would be a lot simpler if the jdk tests had the >>>>>>>>>> hotspot >>>>>>>>>> test library's process tools available. :( >>>>>>>>> We have some, is there an obvious gap? >>>>>>>>> >>>>>>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/e407df8093dc/test/lib/testlibrary/jdk/testlibrary/ >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> Hm, thanks for the info. I should have used this library instead. >>>>>>>> >>>>>>>> Please, stand by for the updated webrev. >>>>>>> I was able to get rid off the JCMD. Using the testlibrary the target >>>>>>> application can recognize its own PID and print it to its stdout. >>>>>>> The >>>>>>> main application then just reads the stdout to parse the PID. No >>>>>>> need >>>>>>> for JCMD any more. >>>>>>> >>>>>>> I could not find a way to remove the dependency on "test.jdk" system >>>>>>> property. According to the jtreg web documentation >>>>>>> (http://openjdk.java.net/jtreg/vmoptions.html#cmdLineOpts) a >>>>>>> "test.java" >>>>>>> system property should be available but in fact is not. But it seems >>>>>>> that the testlibrary uses "test.jdk" system property too. >>>>>>> >>>>>>> The test does not run on OSX because nobody built the launcher >>>>>>> binary :) >>>>>>> I think it is a kind of DIY so I took the liberty of adding a >>>>>>> linux-amd64 launcher while working on the test. >>>>>>> >>>>>>> While working with the test library I realized I was missing a >>>>>>> crucial >>>>>>> feature (at least for my purposes) - waiting for a certain >>>>>>> message to >>>>>>> appear in the stdout/stderr of the launched process. Very often I >>>>>>> need >>>>>>> to wait for the target process to get to certain point before the >>>>>>> test >>>>>>> can be allowed to continue - and the point is indicated by a >>>>>>> message in >>>>>>> stdout/stderr. Currently all the proc tools are designed to work in >>>>>>> "batch" mode - the whole stdout/stderr is captured in strings and >>>>>>> analyzed after the target process died - and are not suitable for >>>>>>> this >>>>>>> kind of usage. >>>>>>> >>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8004926/webrev.01 >>>>>>> >>>>>>>> -JB- >>>>>>>> >>>>>>>>> >>>>>>>>> -Chris. >>>>>>>>> >>>>>>>>>> David >>>>>>>>>> ----- >>>>>>>>>> >>>>>>>>>> On 12/09/2013 1:39 AM, Jaroslav Bachorik wrote: >>>>>>>>>>> Please, review the patch for an intermittently failing test. >>>>>>>>>>> >>>>>>>>>>> The test is a shell test, using files for the interprocess >>>>>>>>>>> synchronization. This leads to intermittent failures. >>>>>>>>>>> >>>>>>>>>>> In order to fix this the test is rewritten in Java - the >>>>>>>>>>> original >>>>>>>>>>> functionality and outputs should be 100% preserved. The patch is >>>>>>>>>>> unfortunately a bit difficult to follow since there is no >>>>>>>>>>> similarity >>>>>>>>>>> between the *.sh and *.java file so one needs to go through the >>>>>>>>>>> new >>>>>>>>>>> source in whole. >>>>>>>>>>> >>>>>>>>>>> The changes in "launcher" files are all about adding >>>>>>>>>>> permissions to >>>>>>>>>>> execute (0755) and as such the webrev shows no differences. >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> >>>>>>>>>>> Issue : JDK-8004926 >>>>>>>>>>> Webrev : http://cr.openjdk.java.net/~jbachorik/8004926/webrev.00 >>>>>>>>>>> >>>>>>>>>>> -JB- >>>>>>>>>>> >>>>>> >>>> >>> >> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
