Serguei, http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.04/
I refactored the debugInit_exit function. Now we have separate exit code for transport error and better readable code. -Dmitry On 2014-08-27 00:22, serguei.spit...@oracle.com wrote: > Dmitry, > > It makes sense to consider a special exit code for transport init error. > Other than that it looks good. > No need to re-review. > > Thanks, > Serguei > > On 8/26/14 10:46 AM, Dmitry Samersoff wrote: >> Serguei, >> >> exit_code set to 1 at ll. 1288 >> >> I thought of refactoring this code for better readability but finally >> decide to keep changes minimal. >> >> -Dmitry >> >> >> On 2014-08-26 21:35, serguei.spit...@oracle.com wrote: >>> Dmitry, >>> >>> This looks good in general. >>> But what error code will be returned in the case of >>> AGENT_ERROR_TRANSPORT_INIT ? >>> Is it Ok to return whatever exit_code we already have or we better >>> return some special error code? >>> >>> Probably, it is Ok to keep the comment at the line 1315 as it is. >>> >>> >>> Thanks, >>> Serguei >>> >>> On 8/26/14 7:47 AM, Dmitry Samersoff wrote: >>>> Staffan, >>>> >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.03/ >>>> >>>> After couple of experiments I end up with simple fix - don't >>>> coredump on >>>> AGENT_ERROR_TRANSPORT_INIT >>>> >>>> Current code don't propagate transport error to upper level, so if we >>>> need fine grained control I'll rewrite transport initialization. >>>> >>>> -Dmitry >>>> >>>> >>>> On 2014-08-25 15:53, Staffan Larsen wrote: >>>>> On 25 aug 2014, at 13:33, Dmitry Samersoff >>>>> <dmitry.samers...@oracle.com >>>>> <mailto:dmitry.samers...@oracle.com>> wrote: >>>>> >>>>>> Staffan, >>>>>> >>>>>> On 2014-08-25 15:26, Staffan Larsen wrote: >>>>>>> Dmitry, >>>>>>> >>>>>>> One problem with this fix is that debugInit_exit() is used from many >>>>>>> places in the JDWP code. For some of these I think it still does >>>>>>> make >>>>>>> sense to receive a core dump for analysis. I agree that when parsing >>>>>>> options, we do not need a core dump. >>>>>> jdwp has explicit option to request a coredump at debugInit_exit(). >>>>>> >>>>>> Is it enough or I should create a more sophisticated fix ? >>>>> I don’t think that is enough. Often a problem will happen and will not >>>>> be reproducible. >>>>> >>>>> Maybe this code: >>>>> >>>>> if ((arg.error != JDWP_ERROR(NONE)) && >>>>> (arg.startCount == 0) && >>>>> initOnStartup) { >>>>> EXIT_ERROR(map2jvmtiError(arg.error), "No transports >>>>> initialized"); >>>>> } >>>>> >>>>> can use some variant of EXIT_ERROR that does not call fatalError() ? >>>>> >>>>> /Staffan >>>>> >>>>>> Actually, I don't think that coredump on every call to >>>>>> jni_FatalError() >>>>>> (see jni.cpp:726) is necessary but this hotspot code is here for 6 >>>>>> years >>>>>> and changing it probably out of scope of this fix. >>>>>> >>>>>> -Dmitry >>>>>> >>>>>> >>>>>>> /Staffan >>>>>>> >>>>>>> >>>>>>> On 20 aug 2014, at 17:55, Dmitry Samersoff >>>>>>> <dmitry.samers...@oracle.com >>>>>>> <mailto:dmitry.samers...@oracle.com>> wrote: >>>>>>> >>>>>>>> Serguei, >>>>>>>> >>>>>>>> After some additional testing I changed the fix a bit. Please take >>>>>>>> a look at new version. >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.02/ >>>>>>>> >>>>>>>> New version reports JVMTI error to stderr. >>>>>>>> >>>>>>>> Also jniFatalError is not referenced any more so I remove it. >>>>>>>> >>>>>>>> -Dmitry >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 2014-08-20 12:08, serguei.spit...@oracle.com wrote: >>>>>>>>> Ok. >>>>>>>>> >>>>>>>>> Thank you for the explanation! Serguei >>>>>>>>> >>>>>>>>> On 8/20/14 1:01 AM, Dmitry Samersoff wrote: >>>>>>>>>> Serguei, >>>>>>>>>> >>>>>>>>>> 1. Historically JDI test-suite had no tests for failed >>>>>>>>>> transport initialization behavior and invalid parameters >>>>>>>>>> handling. >>>>>>>>>> >>>>>>>>>> 2. As a part of JDWP hardening work I added couple of such >>>>>>>>>> tests to OptionTest.java - these tests pass invalid parameters >>>>>>>>>> to dt_socket transport to make sure that transport doesn't >>>>>>>>>> crash (one such crash was discovered and fixed) but just return >>>>>>>>>> non-zero exit code to upper level. >>>>>>>>>> >>>>>>>>>> 3. After fix for JDK-6694099 any non-zero exit code from >>>>>>>>>> transport cause VM to coredump. Dumping multiple cores on busy >>>>>>>>>> machine takes a time so harness kills the test by timeout. >>>>>>>>>> >>>>>>>>>> We can just increase timeout for this test but I don't think >>>>>>>>>> it's a good idea to dump core when invalid parameters passed to >>>>>>>>>> transport. >>>>>>>>>> >>>>>>>>>> So there is the fix. >>>>>>>>>> >>>>>>>>>> 4. After the fix tests for negative parameters will return >>>>>>>>>> non-zero exit code as expected but will not dump the core. >>>>>>>>>> >>>>>>>>>> -Dmitry >>>>>>>>>> >>>>>>>>>> On 2014-08-20 00:54, serguei.spit...@oracle.com wrote: >>>>>>>>>>> Hi Dmitry, >>>>>>>>>>> >>>>>>>>>>> The fix seems to be Ok. Just want to make it clear... This >>>>>>>>>>> fix just changes the bug pattern. It a case of incorrect >>>>>>>>>>> transport parameters the test is still going to fail but >>>>>>>>>>> without crash, right? >>>>>>>>>>> >>>>>>>>>>> Thanks, Serguei >>>>>>>>>>> >>>>>>>>>>> On 8/19/14 12:09 PM, Dmitry Samersoff wrote: >>>>>>>>>>>> Hi Everybody, >>>>>>>>>>>> >>>>>>>>>>>> Please review the fix: >>>>>>>>>>>> >>>>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8049226/webrev.01/ >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>> JDWP call jniFatalError if transport can't be initialized (e.g. wrong >>>>>>>>>>>> parameters) and jniFatalError call os::abort(). Therefor >>>>>>>>>>>> all transport initialization errors cause vm to coredump. >>>>>>>>>>>> >>>>>>>>>>>> I see no reason for debugInit_exit to call jniFatalError so >>>>>>>>>>>> remove this code. >>>>>>>>>>>> >>>>>>>>>>>> -Dmitry >>>>>>>>>>>> >>>>>>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, >>>>>>>> Russia * I would love to change the world, but they won't give me >>>>>>>> the sources. >>>>>> -- >>>>>> Dmitry Samersoff >>>>>> Oracle Java development team, Saint Petersburg, Russia >>>>>> * I would love to change the world, but they won't give me the >>>>>> sources. >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
