Hi Claes,
It looks pretty good.
One question on the following fragment:
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/src/java.base/share/native/libjava/Module.c.udiff.html
+ int valid = 1;
+ for (idx = 0; idx < num_packages; idx++) {
+ jstring pkg = (*env)->GetObjectArrayElement(env, packages, idx);
+ pkgs[idx] = GetInternalPackageName(env, pkg, NULL, 0);
+ if (pkgs[idx] == NULL) {
+ valid = 0;
+ break;
+ }
+ }
+
+ if (valid != 0) {
+ JVM_DefineModule(env, module, is_open, version, location,
+ (const char* const*)pkgs, num_packages);
+ }
+ }
It looks like the module won't be defined if there was an OOM in
processing one of the package names.
The malloc is used only if the supplied buffer size is not enough.
Would it make sense to increase the buffer size from 128 to 256 (or
even 512) at the lines:
119 char buf[128]; 140 char buf[128];
161 char buf[128]; 181 char buf[128];
Nit: it is also better to use a named constant instead.
Thanks, Serguei On 1/6/17 06:23, Claes Redestad wrote:
On 2017-01-06 15:07, Alan Bateman wrote:
On 06/01/2017 13:18, Claes Redestad wrote:
: Updated jdk webrev in-place:
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/
I looked through the latest webrev and it looks quite good. Part of
me regrets introducing JNI code of course but this is startup
motivated. In Module.c then I assume GetInternalPackageName should be
static getInternalPackageName as the scope is just this file (no need
for JNIEXPORT).
Done, will update in place.
Do we have tests that use package names > 128 to test cases where the
buffer on the stack not large enough?
I'm not sure; the malloc'ing path is exercised by DefineModule, but it
would make sense to add a sanity test to each method I guess. /Claes
