This version looks good. Reviewed.
/Staffan
On 27 aug 2014, at 12:51, Dmitry Samersoff <dmitry.samers...@oracle.com> wrote:
> Serguei,
>
> Fixed (in-place, press shift reload)
>
> -Dmitry
>
>
> On 2014-08-27 14:25, serguei.spit...@oracle.com wrote:
>> One more minor comment.
>> The indent in this file must be 4.
>> Could you fix it?
>>
>> Thanks,
>> Serguei
>>
>> On 8/27/14 2:03 AM, Dmitry Samersoff wrote:
>>> Serguei,
>>>
>>> Fixed (in-place, press shift reload)
>>>
>>> -Dmitry
>>>
>>>
>>> On 2014-08-27 12:12, serguei.spit...@oracle.com wrote:
>>>> 1284 enum exit_codes { EXIT_NO_ERRORS = 0, EXIT_TRANSPORT_ERROR,
>>>> EXIT_JVMTI_ERROR };
>>>>
>>>>
>>>> I'd suggest to swap the positions of EXIT_TRANSPORT_ERROR and
>>>> EXIT_JVMTI_ERROR
>>>> in the enum to keep this as compatible to previous behavior as possible.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>
>>>> On 8/27/14 1:08 AM, serguei.spit...@oracle.com wrote:
>>>>> This looks good.
>>>>>
>>>>> Could you, please, remove extra spaces in the following conditions ?:
>>>>>
>>>>> 1291 if (error != JVMTI_ERROR_NONE && docoredump ) {
>>>>> 1302 if ( gdata->jvmti != NULL ) {
>>>>> 1309 if ( error == JVMTI_ERROR_NONE ) {
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 8/27/14 12:36 AM, Dmitry Samersoff wrote:
>>>>>> 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.
