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.
