Oliver Deakin wrote:
Geir Magnusson Jr. wrote:
Gregory Shimansky wrote:
Geir Magnusson Jr. wrote:
I found it - comment below :
Gregory Shimansky wrote:
I've created a patch to classlib which fixes the problem with
drlvm. Now if you could test it with IBM VME, I'll commit it:
Index: modules/luni/src/main/native/luni/shared/luniglob.c
===================================================================
--- modules/luni/src/main/native/luni/shared/luniglob.c (revision
[SNIP]
This puts a bandage on the problem - it seems like we changed the
API of GetSystemProperty() because this code clearly expected not to
get an error if the property wasn't set.
I've read Oliver's comment to r486100 and it didn't look like this
code shouldn't expect an error from GetSystemProperty:
======================================
Previously we just replaced whatever was already in the
org.apache.harmony.boot.class.path property with our own
bootclasspath. We should not assume that this property is empty
before we use it - it depends on the VM's bcp initialisation order
and how it utilises this property internally.
======================================
The "We should not assume" doesn't mean "it should always be this
way" in my understanding.
Right - the thing I was asking if the behavior of GetSystemProperty()
changed.
Looking at Ollie's change in r486100, he seems to think that it's not
an error to not have the property set, but our impl of it does.
That's the issue I'm trying to get to - does the API of
GetSystemProperty() specify that it returns an error code if the
property isn't set?
If so, then Ollie's code is wrong. If not, then it's either that the
API is ambiguous (Ollie's assumption wasn't unreasonable) or our impl
of GetSystemProperty() is wrong.
Apologies for the break - I must admit that I did not test the change on
DRLVM before the commit.
The IBM VME implementation of GetSystemProperty() does not return an
error code in the case of
a non-existent property. I had assumed that this must have been defined
behaviour for that function,
and that it would also work on DRLVM - I guess the API must be ambiguous
here then, since we
appear to have two different outcomes for this case.
I don't think we should treat it as an error if the property is not set.
It makes sense to just return
something that indicates the property is not set (ie NULL) rather than
returning an error code.
We should still check for errors after the GetSystemProperty() call, but
this should not be an
indicator of a non-existent property (as this isn't really an error IMHO).
In this case (and just having seen the API clarification Tim has sent
out) I would suggest
the following patch:
Index: luniglob.c
===================================================================
--- luniglob.c (revision 486569)
+++ luniglob.c (working copy)
@@ -269,14 +269,18 @@
/* Make a string version of the CP separator */
char cpSeparator[] = {(char)hysysinfo_get_classpathSeparator (),
'\0'};
- /* Read current value of bootclasspath property */
+ /* Read current value of bootclasspath property - sets
+ bootstrapClassPath = NULL if the property does not exist */
rcGetProperty = (*vmInterface)->GetSystemProperty (vmInterface,
BOOTCLASSPATH_PROPERTY,
&bootstrapClassPath);
- /* Gregory - no property is found, VM bootclasspath is not
defined */
+ /* There has been an error getting the property - cleanup and
exit */
if (VMI_ERROR_NONE != rcGetProperty)
- bootstrapClassPath = NULL;
+ {
+ returnCode = JNI_ERR;
+ goto cleanup;
+ }
qsort(props, number, sizeof(key_value_pair), props_compare);
Comments?
I like the patch. If we agree in the other thread that GetSystemProperty
should just return NULL, I'll change VMI implementation for drlvm, then
please commit this patch along with Tim's clarification.
--
Gregory
