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.
