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.
