The documentation for hyfile_write says:
* @return Number of bytes written on success, -1 on failure.
* @internal @todo return negative portable return code on failure.
However, the '@todo' is actually done as both the unix and windows
implementations return the result of portLibrary->error_set_last_error(...)
which is the negative portable return code.
Unfortunately, much of the native code depends on the -1 return. For
instance, OSFileSystem.writeImpl has:
jlong result;
result =
(jlong) hyfile_write ((IDATA) fd, (void *) (bytes + offset),
(IDATA) nbytes);
if(result == -1 &&
hyerror_last_error_number() == HYPORT_ERROR_FILE_LOCKED){
throwNewExceptionByName(env, "java/io/IOException", ...);
}
but in the HYPORT_ERROR_FILE_LOCKED error case 'result' is not -1
but the negative portable return code, HYPORT_ERROR_FILE_LOCKED. So
this code is never executed.
Test that cover this have been passing because the result is returned
and checked by java code and a generic exception (without the correct
error message) is thrown instead.
There are several issues to notice here:
1) hyfile_write documentation needs fixing.
2) The -1 checks need reviewing and fixing.
3) The -1 check in this case could be removed *but* it would be better
to replace them with:
if (result < 0) {
throwNewExceptionByName(env, "java/io/IOException",
netLookupErrorString(env, result);
}
so an accurate exception can be thrown (in all cases not just the file
locked case) and remove the wrapping/exception throwing from java code.
(I nearly always prefer this approach - throwing specific exceptions
in natives - but the use of return codes and generic exceptions is
unfortunately rather prevalent.)
4) The jlong 'result' is representing the return code from a portlibrary
call so really it should have a portlibrary type, IDATA in this case.
It should be cast to a java type in the return statement - that is, at
the boundary where we "move" from native to java code. This also seems
quite prevalent (particularly using java types for return values of
syscalls rather than the type described in the documentation for the
syscall). The result is excessive casting of results of calls when
really casting to java types should only happen when making JNI calls or
on return statements. This also makes the code rather hard to review
since you have to look up the correct return type for the syscall to
ensure that the cast is valid.
Regards,
Mark.
